Bugzilla – Full Text Bug Listing
| Summary: | Potential deadlock due synchronization on ConcurrentHashMap | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Martin JANDA <jandam> |
| Component: | Eclipselink | Assignee: | Nobody - feel free to take it <nobody> |
| Status: | NEW --- | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | martin.grebac, michael.f.obrien, tom.ware |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| URL: | https://fisheye2.atlassian.com/browse/eclipselink/trunk/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sequencing/PreallocationHandler.java?r=HEAD#l38 | ||
| Whiteboard: | submitted_patch, concurrency | ||
| Bug Depends on: | |||
| Bug Blocks: | 455043 | ||
>code coordinate is org.eclipse.persistence.internal.sequencing.PreallocationHandler https://fisheye2.atlassian.com/browse/eclipselink/trunk/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/sequencing/PreallocationHandler.java?r=HEAD#l38 >Q1: Why not use the existing intrinsic lock (this) of PreallocationHandler >instead of a new Object (sync) as replacement for the locking object in ConcurrentHashMap? It may be that the lock is protected from use by other threads by making it private >We should also investigate use of the new 1.5 Lock interface which allows us to interrupt locked threads Removing simple_fix annotation. Simple fixes should not require a new regression test. If possible, please give steps that will actually create a deadlock. This is only potential deadlock. So I think it is not possible to reproduce it. Current code depends on internal implementation of ConcurrentHashMap. I use several rules from my experience with multithread programming. a) it is not recommended to synchronize on java.util.concurrent instances - I got deadlock for several times - FindBugs has warning about it - I think there will be some changes in java 7 in j.u.c b) intrinsic lock (this) of PreallocationHandler is dangerous because it is used for internal synchronization of preallocatedSequences but anybody can use instance of PreallocationHandler for synchronization. c) I use 'private final Object sync...' i have 100% control on synchronization -> it would be better to have 'preallocatedSequences' with private access. Because class is package local and not final. It means anybody from package can extend the class and add object to Map with wrong synchronization. --- - I think there should be no specific test for this problem (standard test suite should cover this change) - PreallocationHandler is used only in SequencingManager - I tried to suggest better implementation (small change) to avoid problem that can be very difficult to locate and reproduce - new 1.5 Lock interface isn't necessary here Setting initial target and priority. See the following page for details of the meanings of these fields: http://wiki.eclipse.org/EclipseLink/Development/Bugs/Guidelines Note: Since there is a submitted patch there is a reasonable chance that this bug will move up in the queue. The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink |
Build Identifier: 2.2.0 nightly 2010-10-02 There is synchronization on 'Map<String, Queue> preallocatedSequences' (line 38) that is initialized as ConcurrentHashMap (line 68). This can lead to deadlock because ConcurrentHashMap has own synchronization mechanism. Suggested fix: add new synchronization field: protected Map<String, Queue> preallocatedSequences; + private final Object sync = new Object(); if (sequences == null) { - synchronized (preallocatedSequences) { + synchronized (sync) { Reproducible: Always