Bugzilla – Full Text Bug Listing
| Summary: | Thread safety violations in PDOM objects | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Sergey Prigogin <eclipse.sprigogin> | ||||
| Component: | cdt-indexer | Assignee: | Markus Schorn <mschorn.eclipse> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Markus Schorn <mschorn.eclipse> | ||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | cdtdoug, jamesblackburn+eclipse, malaperle, yevshif | ||||
| Version: | 8.0 | ||||||
| Target Milestone: | 8.0 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Sergey Prigogin
Before I start to change code I'd like to discuss the techniques we should use for achieving thread safety. java.lang.Long: No change needed, Long.value is final and thus the JVM guarantees that it is fully initialized. (http://java.sun.com/docs/books/jls/third_edition/html/memory.html#17.5) Where possible we can make all fields of cached objects final to get the guarantee that a cached value is fully initialized when seen by another thread. Candidates for that are: IFunctionType, PDOMCPPClassScope, PDOMCPPEnumScope, ... long: We can use a Long instead, or make the long volatile. Arrays or other objects with non-final fields: We can introduce an artifical holder object with a final field (similar like Long for long). With that we'd get the same guarantee. Alternatively we would need to use a volatile member to cache such objects. I am not sure what the better approach is. The holder object requires less data-synchronziation from the VM, however it uses more memory (the holder object). (In reply to comment #1) > long: > We can use a Long instead, or make the long volatile. Of course object references to cached objects need to either be: final, volatile or synchronized if accessed from multiple threads. So the reference to the Long should be volatile if you were to make a 'long' volatile. final fields in referenced object aren't transitive so without doing this you could get a stale object-reference on read. > Arrays or other objects with non-final fields: > We can introduce an artifical holder object with a final field (similar like > Long for long). With that we'd get the same guarantee. Alternatively we would > need to use a volatile member to cache such objects. I am not sure what the > better approach is. The holder object requires less data-synchronziation from > the VM, however it uses more memory (the holder object). I know it's an implementation detail, but volatile is pretty efficient on x86 and the basis of the concurrent collections. There's a synchronization cost to writing to a volatile field, but no overhead for reading one: http://gee.cs.oswego.edu/dl/jmm/cookbook.html (In reply to comment #2) > Of course object references to cached objects need to either be: final, > volatile or synchronized if accessed from multiple threads. So the reference to > the Long should be volatile if you were to make a 'long' volatile. > final fields in referenced object aren't transitive so without doing this you > could get a stale object-reference on read. In general you'd be right, however we are dealing with cached values that never change. So when a thread sees the reference to the Long, all it needs to be sure about is, that the field of the Long (Long.value) is actually initialized correctly, it does not have to worry whether another Long was assiged to the reference, because it would just contain the same value. The situation is the same for all PDOM objects, because we do not have concurrent access while the indexer writes to the PDOM. (In reply to comment #3) > In general you'd be right, however we are dealing with cached values that never > change. So when a thread sees the reference to the Long, all it needs to be > sure about is, that the field of the Long (Long.value) is actually initialized > correctly, it does not have to worry whether another Long was assiged to the > reference, because it would just contain the same value. If the code is something like Long field; long getField() { if (field == null) { field = new Long(some_value); } return field.longValue(); } then field has to be volatile to make sure that the final field Long.value is fully initialized before other threads can see non-null value of field. (In reply to comment #4) > then field has to be volatile to make sure that the final field Long.value is > fully initialized before other threads can see non-null value of field. That's not true. The final qualifier on a member guarantees that correctly initialized object (i.e. one that doesn't leak a reference to `this` in its constructor) is consistently published to all threads. i.e. if the thread can see the object-reference, it sees a correctly published object. See 17.5 of: http://java.sun.com/docs/books/jls/third_edition/html/memory.html "Final fields also allow programmers to implement thread-safe immutable objects without synchronization. A thread-safe immutable object is seen as immutable by all threads, even if a data race is used to pass references to the immutable object between threads. This can provide safety guarantees against misuse of an immutable class by incorrect or malicious code. Final fields must be used correctly to provide a guarantee of immutability." (In reply to comment #5) You're right. I missed the "The initial load (i.e., the very first encounter by a thread) of a final field cannot be reordered with the initial load of the reference to the object containing the final field" rule from http://gee.cs.oswego.edu/dl/jmm/cookbook.html. (In reply to comment #6) Yep, the nice thing about the Java 5 MM is that it basically works how you expect :), hopefully making many concurrency bugs a thing of the past. Ok I think I'll use the following strategy for the cached objects in pdom nodes: No changes needed for: 1) Fundamental types <= 32-bit (write is atomic) 2) Objects with final fields, only (initialization is guaranteed) Use volatile for: 1) Type 'long' 2) Array types 3) Cached objects with non-final fields. Created attachment 192339 [details]
Adjusts all fields of the PDOMNodes
I looked at all subclasses of PDOMNode, at PDOMCPP*Scope, PDOMMacro* and PDOMFile.
I think that is all we need to do.
Thanks for raising the issue and participating in the discussion. Please reopen the bug if you can think of further problems. *** cdt cvs genie on behalf of mschorn *** Bug 341440: Thread safety for PDOM objects. [*] PDOMCEnumeration.java 1.16 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/c/PDOMCEnumeration.java?root=Tools_Project&r1=1.15&r2=1.16 [*] PDOMCPPFunctionSpecialization.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPFunctionSpecialization.java?root=Tools_Project&r1=1.19&r2=1.20 [*] PDOMCPPClassScope.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassScope.java?root=Tools_Project&r1=1.14&r2=1.15 [*] PDOMCPPDeferredClassInstance.java 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPDeferredClassInstance.java?root=Tools_Project&r1=1.32&r2=1.33 [*] PDOMCPPNamespace.java 1.58 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPNamespace.java?root=Tools_Project&r1=1.57&r2=1.58 [*] PDOMCPPFunction.java 1.54 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPFunction.java?root=Tools_Project&r1=1.53&r2=1.54 [*] PDOMCPPClassSpecialization.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassSpecialization.java?root=Tools_Project&r1=1.31&r2=1.32 [*] PDOMCPPEnumScope.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPEnumScope.java?root=Tools_Project&r1=1.3&r2=1.4 [*] PDOMCPPSpecialization.java 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPSpecialization.java?root=Tools_Project&r1=1.24&r2=1.25 [*] PDOMCPPTemplateTypeParameter.java 1.24 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPTemplateTypeParameter.java?root=Tools_Project&r1=1.23&r2=1.24 [*] PDOMCPPUnknownClassInstance.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPUnknownClassInstance.java?root=Tools_Project&r1=1.19&r2=1.20 [*] PDOMCPPEnumeration.java 1.26 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPEnumeration.java?root=Tools_Project&r1=1.25&r2=1.26 [*] PDOMCPPClassTemplate.java 1.47 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassTemplate.java?root=Tools_Project&r1=1.46&r2=1.47 [*] PDOMCPPUsingDeclaration.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPUsingDeclaration.java?root=Tools_Project&r1=1.9&r2=1.10 [*] PDOMCPPClassType.java 1.87 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassType.java?root=Tools_Project&r1=1.86&r2=1.87 [*] PDOMCPPLinkage.java 1.149 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPLinkage.java?root=Tools_Project&r1=1.148&r2=1.149 [*] PDOMCPPUnknownClassType.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPUnknownClassType.java?root=Tools_Project&r1=1.17&r2=1.18 [*] PDOMCPPUnknownScope.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPUnknownScope.java?root=Tools_Project&r1=1.2&r2=1.3 [*] PDOMCPPClassTemplatePartialSpecializationSpecialization.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPClassTemplatePartialSpecializationSpecialization.java?root=Tools_Project&r1=1.5&r2=1.6 [*] PDOMCPPTemplateNonTypeParameter.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPTemplateNonTypeParameter.java?root=Tools_Project&r1=1.20&r2=1.21 [*] PDOMCPPFunctionTemplate.java 1.29 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPFunctionTemplate.java?root=Tools_Project&r1=1.28&r2=1.29 [*] PDOMCPPTemplateTemplateParameter.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/cpp/PDOMCPPTemplateTemplateParameter.java?root=Tools_Project&r1=1.8&r2=1.9 [*] PDOMLinkage.java 1.74 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMLinkage.java?root=Tools_Project&r1=1.73&r2=1.74 [*] PDOMNode.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMNode.java?root=Tools_Project&r1=1.31&r2=1.32 [*] PDOMNamedNode.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMNamedNode.java?root=Tools_Project&r1=1.17&r2=1.18 [*] PDOMFile.java 1.64 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMFile.java?root=Tools_Project&r1=1.63&r2=1.64 [*] CFunctionType.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/c/CFunctionType.java?root=Tools_Project&r1=1.10&r2=1.11 [*] CPPUnknownScope.java 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPUnknownScope.java?root=Tools_Project&r1=1.32&r2=1.33 [*] AbstractCPPClassSpecializationScope.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/AbstractCPPClassSpecializationScope.java?root=Tools_Project&r1=1.13&r2=1.14 [*] CPPFunctionType.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPFunctionType.java?root=Tools_Project&r1=1.18&r2=1.19 |