It has been my experience that S3077 always generates false positives for the projects on which I work. The intent of the rule appears to be to indicate that using “volatile MyObject myRef” does not guarantee that different threads will see updates to the object referenced by “myRef”, which is correct. However, in my experience, use of “volatile” has only ever been used to guarantee that different threads will see updates to myRef, itself, not to the object it references. (On the other hand, I have found that code that uses “volatile” for primitive, mutable values other than boolean is almost always incorrect.)
The workaround, to use AtomicReferences, obfuscates the code.
Could you provide some small code examples of good findings and false-positive about S3077?
My view of this rule is: Usage of volatile is related to a multi-threads context. If a mutable object is shared, “volatile” will not help to prevent concurrent modifications, so it’s good to inform the developer.
Unfortunately, the rule is not able to distinguish if the referenced object is mutable (at risk) or not. And with a mutable object, using AtomicReference does not prevent underlying concurrent modifications.
One possible change for this rule would be to focus only on arrays. Because java arrays are always mutable, and java.util.concurrent.atomic.Atomic*Array provides good helpers to manipulate arrays of immutable objects safely.
Just returned from vacation. Suppose I want to set a variable and have other threads retrieve the variable as soon as it has been set. That’s when I would typically use “volatile” to guarantee that other threads reading the variable see the latest value:
As multiple threads may invoke makeNewObject() or clearObject(), “object” has been declared “volatile” so that getObject() always returns the last object that was set. Note: the validity of this code has nothing to do with whether or not SomeObject is mutable; that is an independent issue. When dealing with non-primitive objects, I have never seen a case where a developer declared “object” to be volatile as an (erroneous) means to ensure that updates to the referenced object were visible in other threads; I have only ever seen it used as above - a completely legitimate use.
When sonar reports these, we are forced to either change to AtomicReference or disable sonar for this declaration. I prefer developers to not get in the habit of disabling sonar, but this constant unnecessary warning leads us to do just that.
Hey, it’s been very quiet here for the last 2 years, but the problem still exists, so I want to resurrect the thread with a few suggestions.
As already mentioned in other treads as well immutable types are not a problem. Thread safe types should not be reported too.
How about having ThreadSafe and Immutable annotations which are taken into account during the code analysis for this rule. If I have private volatile SomeObject object;, where SomeObject is marked with any of the annotations above, then the rule should not detect any problems.
Since SomeObject might be a 3rd party class which we cannot annotate, we should also have some external annotation mechanism like this one in IntelliJ, which gives me the mechanism for controlling the above.
What do you think about this? Or do you maybe have another solution, yet?
The current rule implementation already supports the @javax.annotation.concurrent.Immutable and @javax.annotation.concurrent.ThreadSafe annotations. Unfortunately, we currently do not support external annotations - although I will take it away as a topic for discussion. For completeness, we do have an open ticket for improving this rule’s accuracy.