DistributedStateCacheImpl not thread safe?

[glassfishrobot]

Hello,

tho i see several issues, i concentrate myself only on one, the most obvious:

private static final Map<String, DistributedStateCacheImpl> ctxCache = new HashMap<String, DistributedStateCacheImpl>(); //return the only instance we want to return static DistributedStateCache getInstance(final String groupName) { DistributedStateCacheImpl instance; if (ctxCache.get(groupName) == null)

{ instance = new DistributedStateCacheImpl(groupName); ctxCache.put(groupName, instance); }

else

{ instance = ctxCache.get(groupName); }

return instance; }

I think, shoal should take care of concurrency issues on ctxCache as well.

Why not using ConcurrentMap as well? Maybe like this (which works fine, as long instantiating an instance is not a heavy operation):

ConcurrentMap<String, DistributedStateCacheImpl> ctxCache = ...;

static DistributedStateCache getInstance(final String groupName) { DistributedStateCacheImpl instance = ctxCache.get(groupName); if (instance == null) { instance = new DistributedStateCacheImpl(groupName);

// put our mapping only, if no other mapping has been put already DistributedStateCacheImpl otherInstance = ctxCache.putIfAbsent(groupName, instance);

// there was another mapping, use that one instead of ours if (otherInstance != null)

{ instance = otherInstance; }

} return instance; }

Other issues:

• GSMContext ctx is not guarded
• firstSyncDone is not guarded

Right now it seems, someone has to synchronize on its own. At least this should be reflected in the javadoc:

"The implemenation itself is not thread-safe"

What do you think?

Environment

Operating System: All Platform: All

Affected Versions

[current]

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/shoal/issues/79
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @jfialli
• Original Issue Closed By:@glassfishrobot

[glassfishrobot]

@glassfishrobot Commented Reported by okrische

[glassfishrobot]

@glassfishrobot Commented shreedhar_ganapathy said: Excellent observation! Could you also file cases for other issues you see with DistributedCache ? Are you interested in contributing fixes? We always will welcome those.

[glassfishrobot]

@glassfishrobot Commented okrische said: You want me to submit a patch on this issue? I can do, will you append it then to the branch?

[glassfishrobot]

@glassfishrobot Commented shreedhar_ganapathy said: We'd be happy to do that. If you send the contributor agreement, you will have commit access to check in your patch after review and some testing.

Thanks Shreedhar

[glassfishrobot]

@glassfishrobot Commented okrische said: Created an attachment (id=11) patch to fix concurrency issues for this class only

[glassfishrobot]

@glassfishrobot Commented okrische said: Okay, here some comments to the patch:

• Logger is final static instead of just final. Saves one reference per created instance of DistribuedStateCacheImpl

• cache changed from Map to ConcurrencyHashMap to fix the concurrency issue

• ctx changed to an AtomicReference ctxRef, since ctx will be set at runtime by the first thread, who enters the method to read ctx -> concurrency issue

• firstSyncDone changed to volatile, since it can be changed at runtime by the first thread, who does the sync -> concurrency issue

[glassfishrobot]

@glassfishrobot Commented okrische said: Ups.

-> meant "ctxCache", not cache

[glassfishrobot]

@glassfishrobot Commented @jfialli said: Thanks for the patch. I will verify the changes against our internal Shoal tests. If all checks out, I will update this issue, requesting you to check the change in.

I will update my status on confirming this patch by Monday.

[glassfishrobot]

@glassfishrobot Commented @jfialli said: Patch checked in. Will be included in next shoal-integration into application server.

[glassfishrobot]

@glassfishrobot Commented File: DistributedStateCacheImpl-Diff.txt Attached By: okrische

[glassfishrobot]

@glassfishrobot Commented This issue was imported from java.net JIRA SHOAL-79

[glassfishrobot]

@glassfishrobot Commented Marked as fixed on Wednesday, June 23rd 2010, 4:11:06 am