PagingManagerImpl::getPageStore

[clebert.suconic]

This comment was added into PagingManagerImpl:

 //FIXME - this is not thread safe
 public PagingStore getPageStore(final SimpleString storeName) throws Exception
 {
 PagingStore store = stores.get(storeName);

 if (store == null)
 {
 store = newStore(storeName);

 PagingStore oldStore = stores.putIfAbsent(storeName, store);

 if (oldStore != null)
 {
 store = oldStore;
 }

 store.start();
 }

 return store;
 }

stores is a ConcurrentHashMap, and the method is using putIfAbsent, getting the currentStore case it was already set.

I don't think we need a synchronization here, since this method is used every time page is called. The idea was to use the ConcurrentHashmap to avoid the synchronized block.

[clebert.suconic]

I made several tests around this method...

you could have eventually few instances created and imediatly destroyed during an initial ramp up, but after the initial load... and the ConcurrentHashMap aways gave enough protection on the Store.

Well... But anyway I have created a new method synchronized createPageStore on the Chunk Branch which is aways used in places where I don't need concurrent calls over the same store. When we create the queue now, the pageStore will be also created.

[timfox]

One problem with that code is that in case of race start() will be called on the old store even if it's already started, i.e. it should read:

[clebert.suconic]

"timfox" wrote:
One problem with that code is that in case of race start() will be called on the old store even if it's already started, i.e. it should read:

start is synchronized, and it will return immediately if already started.

But that won't happen after my merge from chunk. All the stores are either started when the server is started, or when the queue is created.

[timfox]

Another reason why it is not threadsafe:

store = newStore(storeName);

With a race you could get two stores being created, and one throw away. When the store is created it creates files on disk. So you'd get two lots of dirs created on disk with the same name which would fail.

[clebert.suconic]

Well.. that is a problem I agree.
The directory creation was supposed to be done inside the start.

And start should be synchronized (as it is) making all the threads wait until this was done.