Community
Participate
Working Groups
Build Identifier: 3.7 A lot of synchronization in the jdt.core.dom package is done wrongly with the double-checked locking pattern which is not working correctly. See http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html. I have fixed the affected classes and will attach a patch. Reproducible: Always
Created attachment 182178 [details] The patch solving the synchronization problem
Thanks for pointing out this latent bug, which has been there from the early days of Eclipse (c. 2001). I'm likely to have introduced the same bug in other code I've written - so all parts of the JDT AST implementation that does lazy init should be checked. I find it interesting that this bug has not shown itself. Why? Is it because the incidence of concurrent access to an AST is rare? Is it because the JVMs in use are not attempting the kinds of compiler optimizations that would cause the code to fail? Is it because the code is not being run on multi-processor hardware? Declaring all those getter methods as synchronized will affect performance. We also need to be concerned about introducing possible deadlocks. Eclipse 3.7 can safely assume Java 5. As described in the "Fixing Double-Checked Locking using Volatile" section of the document mentioned in reference 1, another option for Java 5 or later is to declare the fields volatile (but leave the methods unsynchronized. This will also affect performance. It would be good to understand which approach will have the lesser performance impact in practice, bearing in mind that different JVMs might suffer differently.
This is interesting. Thanks for the patch. We need to check what other places in the compiler code we have used the same pattern. I guess adding the synchronised modifier seems the best approach to me. Srikanth, what's your opinion on this? How can we gauge the performance impact with this change?
(In reply to comment #3) > I guess adding the synchronized modifier seems the best approach to me. > Srikanth, what's your opinion on this? How can we gauge the performance impact > with this change? I would check both ways to fix it: synchronizing the method or making fields volatile. We need to check the performance impact of both and select the best one. Ayushman, could you please take care of this ? Thanks.
(In reply to comment #3) >[..] As a side note, its not fully clear to me why the double locking mechanism wont work with Java in a platform-independent way (as written in the above document). Java's spec to generate code is a standard that will be followed, and i'm sure javac or a javac-compatible compiler will make sure that proper code is generated and that the generated code doesnt break the thread-safetyness. Are we sure that the current java memory model allows generation of improper bytecode for the double-locking mechanism, or are we just doing this because the above research document says so? The very fact that we never ran into problems makes it seem that the java memory model specifies generation of good code for the double checking mechanism.
Ayushman, please take care of this for 3.7.
I tag it as 3.7M7. Please make sure one of the recommended fixes end up being released.
(In reply to comment #7) > I tag it as 3.7M7. Please make sure one of the recommended fixes end up being > released. Olivier, we can't use volatile currenly because its a 1.5 feature. Making all methods synchronized might hit performance (which may also affect clients since this is in the DOM ast nodes). Anyway, we have never really seen any problem in this area till yet. So I propose to defer this till we migrate to 1.5 when we can make the fields volatile. What do you think?
(In reply to comment #8) > (In reply to comment #7) > > I tag it as 3.7M7. Please make sure one of the recommended fixes end up being > > released. > > Olivier, we can't use volatile currenly because its a 1.5 feature. This is not correct, volatile has been in the language well before 1.5 > this is in the DOM ast nodes). Anyway, we have never really seen any problem in > this area till yet. Don't disagree with the above though.
Let's target 3.8. This will be done at the same time we convert JDT/Core APIs to 1.5.
(In reply to comment #9) >[..] > This is not correct, volatile has been in the language well before 1.5 Only in name. The semantics of volatile keyword have only been strengthened from 1.5 onwards. It has issues in 1.4. See the 4th code snippet at: http://en.wikipedia.org/wiki/Double-checked_locking
(In reply to comment #11) > (In reply to comment #9) > >[..] > > This is not correct, volatile has been in the language well before 1.5 > > Only in name. The semantics of volatile keyword have only been strengthened > from 1.5 onwards. It has issues in 1.4. Thanks for the pointer Ayush, I hadn't known this.
We still haven't moved to 1.5, so moving it out of 3.8
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
