Community
Participate
Working Groups
In "org.eclipse.jdt.core.dom", the "Double-Checked Locking" idiom is used multiple times, but without the corresponding lazy initialized fields being "volatile". A field that is lazy initialized via the "Double-Checked Locking" idiom has to be "volatile" to be multithreaded-safe. Otherwise, another thread may get a partial initialized field since the initialization of a non-volatile non-primitiv field is not guaranteed to be atomic. The double-check idiom wasn't guaranteed to work until Java 1.5. Since Java 1.5 the "volatile" keyword ensures that multiple threads handle the singleton instance correctly. In "org.eclipse.jdt.core.dom", at least some of the via the double-check idiom lazy initialized fields have been introduced before Java 1.5. See: - https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java - http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html For example, org.eclipse.jdt.core.dom.ForStatement: public class ForStatement extends Statement { // ... private Statement body = null; // ... public Statement getBody() { if (this.body == null) { // lazy init must be thread-safe for readers synchronized (this) { if (this.body == null) { preLazyInit(); this.body = new Block(this.ast); postLazyInit(this.body, BODY_PROPERTY); } } } return this.body; } // ... } ... has to be (with "volatile" and with "localRef" to access the "volatile" field only once for better performance): public class ForStatement extends Statement { // ... private volatile Statement body = null; // ... public Statement getBody() { Statement localRef = this.body; if (localRef == null) { // lazy init must be thread-safe for readers synchronized (this) { localRef = this.body; if (localRef == null) { preLazyInit(); this.body = localRef = new Block(this.ast); postLazyInit(this.body, BODY_PROPERTY); } } } return localRef; } // ... } Or do I miss something?
Sarika, can you please have a look?
You can find a gerrit for that at https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/171945
(In reply to Carsten Hammer from comment #2) > You can find a gerrit for that at > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/171945 Yes, exactly. But why did you abandon it?
@Holger, Where do you see these AST conversion being called in a multi threaded environment? We need to understand if we really face this issue or it is just by code inspection we see this as an issue.
(In reply to Sarika Sinha from comment #4) > @Holger, > Where do you see these AST conversion being called in a multi threaded > environment? > > We need to understand if we really face this issue or it is just by code > inspection we see this as an issue. The comments say: // lazy init must be thread-safe for readers The "Double-Checked Locking" idiom is used incorrectly here. Maybe it is no longer needed, but then it has to be removed.
(In reply to Holger Voormann from comment #3) > (In reply to Carsten Hammer from comment #2) > > You can find a gerrit for that at > > https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/171945 > > Yes, exactly. But why did you abandon it? Oh, I still think the gerrit is basically right (while there is a slightly nicer coding pattern that is equally right) but after experience in https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/173575 and https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/173560 I felt there is no chance to get that changed and gave up for the moment. It is not so important at the moment for me. Besides just pointing to publications regarding wrong and right coding patterns are not seen as valid measure to change coding in general in the jdt communiy. You have to create a failing junit test to prove it is wrong especially as non-commiter and even then it can be difficult. I have not the time currently to prepare all this. And there are no open bugzillas directly related to this issue which is a very convincing argument imho. Feel free to pick it up.
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. If you have further information on the current state of the bug, please add it. 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.
