Issue Details (XML | Word | Printable)

Key: NUCCORE-553
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Blocker Blocker
Assignee: Erik Bengtson
Reporter: Fernando Padilla
Votes: 0
Watchers: 2
Operations

If you were logged in you would be able to see more operations.
DataNucleus Core

Concurrency Bug: unsynchronized WeakHashMap in OIDFactory

Created: 27/Jul/10 08:40 AM   Updated: 20/Oct/10 09:29 AM   Resolved: 12/Sep/10 10:23 AM
Component/s: None
Affects Version/s: None
Fix Version/s: 2.2.0.m2


 Description  « Hide
Our systems keep hanging, and we have lots of threads stuck on this stack trace:

   java.lang.Thread.State: RUNNABLE
        at java.util.WeakHashMap.get(WeakHashMap.java:355)
        at org.datanucleus.identity.OIDFactory.getInstance(OIDFactory.java:54)
        at org.datanucleus.store.mapped.mapping.OIDMapping.getObject(OIDMapping.java:125)
        at org.datanucleus.store.rdbms.query.PersistentClassROF.getObject(PersistentClassROF.java:360)
        at org.datanucleus.store.rdbms.query.ResultClassROF.getObject(ResultClassROF.java:272)
        at org.datanucleus.store.rdbms.query.ForwardQueryResult.nextResultSetElement(ForwardQueryResult.java:135)
        at org.datanucleus.store.rdbms.query.ForwardQueryResult$QueryResultIterator.next(ForwardQueryResult.java:302)
...
..
.


I did a yahoo search and found the mention below, that states that they ran into it while using WeakHashMap while not properly synchronized. And this is the case here.

http://jira.codehaus.org/browse/XSTR-584


So we simply need to add synchronization around the WeakHashMap access.


ps - I actually wonder, if using a WeakHashMap to cache OIDs is actually beneficial. Just a hypothesis, that we could/should test. With the latest GCs it's sometimes better to simply create short lived objects, rather than try managing pools. Also, I noticed that to generate the Key for the cache, pretty much does the same work (concat class/oid) as what the constructor for OIDImpl (and OIDImplKodo that we use) does. So it doesn't seem to save any work what so ever. Just add another single point of synchronization and bottleneck... I propose to simply get rid of the caching. :)

Sort Order: Ascending order - Click to sort in descending order
Fernando Padilla added a comment - 27/Jul/10 08:49 AM
I wonder if we could make the OIDFactory non-static, and pluggable, so we can more easily override it..

Erik Bengtson added a comment - 12/Sep/10 10:23 AM
oids no longer cached

Chris Colman added a comment - 14/Sep/10 06:54 PM
I discovered a while back that 'weak' caching is virtually not useful in anycase. 'Weakly' cached objects are almost always immediately GCed if the system isn't busy:

http://www.jpox.org/servlet/forum/viewthread_thread,5779#31236