A common misconception is that shared references to immutable objects are immediately visible across multiple threads as soon as they are updated. For example, a developer can mistakenly believe that a class containing fields that refer only to immutable objects is itself immutable and consequently thread-safe.
Section 14.10.2, "Final Fields and Security," of Java Programming Language, Fourth Edition [JPL 2006] states:
The problem is that, while the shared object is immutable, the reference used to access the shared object is itself shared and often mutable. Consequently, a correctly synchronized program must synchronize access to that shared reference, but often programs do not do this, because programmers do not recognize the need to do it.
References to both immutable and mutable objects must be made visible to all the threads. Immutable objects can be shared safely among multiple threads. However, references to mutable objects can be made visible before the objects are fully constructed. TSM03-J. Do not publish partially initialized objects describes object construction and visibility issues specific to mutable objects.
Noncompliant Code Example
This noncompliant code example consists of the immutable Helper
class:
// Immutable Helper public final class Helper { private final int n; public Helper(int n) { this.n = n; } // ... }
and a mutable Foo
class:
final class Foo { private Helper helper; public Helper getHelper() { return helper; } public void setHelper(int num) { helper = new Helper(num); } }
The getHelper()
method publishes the mutable helper
field. Because the Helper
class is immutable, it cannot be changed after it is initialized.
Furthermore, because Helper
is immutable, it is always constructed properly before its reference is made visible, in compliance with TSM03-J. Do not publish partially initialized objects. Unfortunately, a separate thread could observe a stale reference in the helper
field of the Foo
class.
Compliant Solution (Synchronization)
This compliant solution synchronizes the methods of the Foo
class to ensure that no thread sees a stale Helper
reference:
final class Foo { private Helper helper; public synchronized Helper getHelper() { return helper; } public synchronized void setHelper(int num) { helper = new Helper(num); } }
The immutable Helper
class remains unchanged.
Compliant Solution (volatile
)
References to immutable member objects can be made visible by declaring them volatile:
final class Foo { private volatile Helper helper; public Helper getHelper() { return helper; } public void setHelper(int num) { helper = new Helper(num); } }
The immutable Helper
class remains unchanged.
Compliant Solution (java.util.concurrent
Utilities)
This compliant solution wraps the mutable reference to the immutable Helper
object within an AtomicReference
wrapper that can be updated atomically:
final class Foo { private final AtomicReference<Helper> helperRef = new AtomicReference<Helper>(); public Helper getHelper() { return helperRef.get(); } public void setHelper(int num) { helperRef.set(new Helper(num)); } }
The immutable Helper
class remains unchanged.
Risk Assessment
The incorrect assumption that classes that contain only references to immutable objects are themselves immutable can cause serious thread-safety issues.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
VNA01-J | Low | Probable | Medium | P4 | L3 |
Automated Detection
Some static analysis tools are capable of detecting violations of this rule.
Tool | Version | Checker | Description |
---|---|---|---|
ThreadSafe | 1.3 | CCE_SL_INCONSISTENT | Implemented |
SonarQube | 9.9 | S2886 | Getters and setters should be synchronized in pairs |
11 Comments
Dhruv Mohindra
Regarding combining this guideline with CON00/CON26 and so on -
CON00 describes the utility of
volatile
. In particular,1) It ensures that references to immutable objects are immediately made visible (second to last CS in CON00)
This is essentially the same as the recommendation of this guideline. Just that this guideline also suggests alternatives to using volatile. CON00 cannot have a CS with synchronization because it is exclusively for volatile's uses.
2) Member objects that are thread-safe (not necessarily immutable) are properly published "the first time". (last CS of CON00)
This use of volatile in CON00 is much weaker than the approaches suggested in TSM03-J. Do not publish partially initialized objects to the extent that I would almost not recommend using
volatile
to publish anything but immutable objects.In summary, both the visibility and partial initialization issues can be solved using alternative ways which are discussed in separate guidelines (CON09 and CON26).
This guideline cannot be combined with CON26 because it is a visibility issue and NOT a partial object construction issue. Immutable objects are guaranteed to be properly constructed before they are made visible.
David Svoboda
I'll agree that CON26-J is about an object being only partially-constructed when viewed by another thread. And CON09-J is about an object being fully-constructed, but not fully visible when viewed by another thread.
However, while these are distinct problems, they both stem from the same fundamental problem: the lack of a happens-before relation between an object being constructed and its first subsequent read. The solutions are the same.
Are there any NCCE/CS examples we can add to CON09 to further distance itself from CON26? If not, I'd still recommend merging this into CON26 (while expanding the text & title of CON26 to include visibility issues.)
Dhruv Mohindra
David, my recent focus has been to try and separate things as much as possible so that it can help in automatic analysis. Since immutable objects are always fully constructed I am not quite sure how they belong to CON26. I think the
initialize()
method in this guideline is causing some confusion. It should be renamed tosetHelper()
. Essentially, this is about setting the helper and not ensuring that thehelper
itself is properly constructed.I could very well use the example of an immutable
String
instead of helper to make this point. This is also not about ensuring that class Foo is constructed properly. It always is because of the default constructor. These guidelines are close but this does not fit there (note that this guideline actually forked out from CON26). If you put this in CON26, CON26 might lose its focus. Moreover, we would be unable to reference this guideline from other places. CON26 would then have two similar but different issues.Robert Seacord (Manager)
Maybe it's because I'm reading these front to back, but to me this is very similar to the other visibility rule VNA00-J. Ensure visibility when accessing shared primitive variables except it talks about references and not primitives.
I could see combining them, or at least making them adjacent (grouping the visibility rules together).
I would also like to eliminate the synchronization and volatile solutions as this has already been done to death. I would just mention these solutions in passing and reference VNA00-J. Ensure visibility when accessing shared primitive variables. We don't really need to explain every possible compliant solution to every noncompliant code example in every case.
Dhruv Mohindra
Yes, this is similar to CON00's issue. I think all this material was under one guideline when I started out but eventually things got separated out to favor automatic analysis. This guideline also says that an object that has immutable members may not be immutable which convinced me to keep it separate.
I agree with just mentioning the CSs as trivial as these from this point onwards, but am not sure about David's opinion.
David Svoboda
I'm not convinced that the 1st NCCE complies with CON26-J, even though it claims to do so. CON26-J does not seem to have a CS where Helper is immutable but has no other safety mechanisms (eg volatile or final itself).
I suspect the NCCE does in fact comply, but if so, then CON26-J should simplify its set of CS's. And technically 'it is always properly constructed before its reference is made visible' is false, CON26-J guarantees that if a thread gets a non-null Helper object, it will also get a legit value for
x
.WRT abridging, I think we should still have 3 CS headings, in the order: atomic, volatile, synchronized. The synchronized section doesn't need a code sample; we can just have it say "See CON00-J's synchronized CS for an illustration of how to do this". We could possibly do the same for the volatile CS. I think we need to keep the AtomicReference CS, just to have solid code for at least one CS, and b/c it is significantly distinct from the CS's in CON00-J.
Also, when we renumber the rules, I would like us to make 'subgroups', based on subject. Or even partition the Concurrency section altogether; it has grown so huge.
Dhruv Mohindra
Let me put it this way: CON26 is compliant with this guideline and this guideline is compliant with CON26. Agreed? CON26 should probably say this in CS(immutable object - final fields, volatile ref) where the line starts with "Furthermore, if the helper field is declared ...".
Immutable objects are guaranteed to be properly constructed before they are made visible. What is false here?
I think we should either eliminate all CSs and keep one with brief descriptions of the others with xrefs or simply keep all because they aren't too long.
David Svoboda
OK, I'm convinced the NCCE complies with CON26-J since Helper is immutable. If CON26-J cannot be violated using immutable objects, we may want to clean up CON26-J as it has several CS's involving immutable objects.
I suspect the three of us need to sit down and plan which CS's here can be removed. It sounds like none of us has any technical problems with the CSs.
Dhruv Mohindra
I would be happy to eliminate Compliant Solution (immutable object - factory method, volatile reference) from COn26 which is the extra one and leave a line about it in Compliant Solution (immutable object - final fields, volatile reference). Then the latter CS can be named - immutable object, volatile ref.
Furthermore, this guideline could either show how to create an immutable using the public static factory approach or simply mention it in the NCE.
Pennie Walters
In the text below, "them" is vague and needs to be clarified:
Compliant Solution (
volatile
)References to immutable member objects can be made visible by declaring them
volatile
.Also, the Automated Detection is not complete. It's marked TODO.
Dhruv Mohindra
Should probably be: "References to immutable member objects can be made visible by declaring the corresponding fields as volatile".
You may neglect the automated detection section because we are not including this information right now.