Reading a shared primitive variable in one thread may not yield the value of the most recent write to the variable from another thread. Consequently, the thread may observe a stale value of the shared variable. To ensure the visibility of the most recent update, either the variable must be declared volatile or the reads and writes must be synchronized.

Declaring a shared variable volatile guarantees visibility in a thread-safe manner only when both of the following conditions are met:

The first condition can be relaxed when you can be sure that only one thread will ever update the value of the variable [Goetz 2006]. However, code that relies on a single-thread confinement is error prone and difficult to maintain. This design approach is permitted under this rule but is discouraged.

Synchronizing the code makes it easier to reason about its behavior and is frequently more secure than simply using the volatile keyword. However, synchronization has somewhat higher performance overhead and can result in thread contention and deadlocks when used excessively.

Declaring a variable volatile or correctly synchronizing the code guarantees that 64-bit primitive long and double variables are accessed atomically. For more information on sharing those variables among multiple threads, see VNA05-J. Ensure atomicity when reading and writing 64-bit values.

Noncompliant Code Example (Non-volatile Flag)

This noncompliant code example uses a shutdown() method to set the nonvolatile done flag that is checked in the run() method:

final class ControlledStop implements Runnable {
  private boolean done = false;
 
  @Override public void run() {
    while (!done) {
      try {
        // ...
        Thread.currentThread().sleep(1000); // Do something
      } catch(InterruptedException ie) { 
        Thread.currentThread().interrupt(); // Reset interrupted status
      } 
    } 	 
  }

  public void shutdown() {
    done = true;
  }
}

If one thread invokes the shutdown() method to set the flag, a second thread might not observe that change. Consequently, the second thread might observe that done is still false and incorrectly invoke the sleep() method. Compilers and just-in-time compilers (JITs) are allowed to optimize the code when they determine that the value of done is never modified by the same thread, resulting in an infinite loop.

Compliant Solution (Volatile)

In this compliant solution, the done flag is declared volatile to ensure that writes are visible to other threads:

final class ControlledStop implements Runnable {
  private volatile boolean done = false;
 
  @Override public void run() {
    while (!done) {
      try {
        // ...
        Thread.currentThread().sleep(1000); // Do something
      } catch(InterruptedException ie) { 
        Thread.currentThread().interrupt(); // Reset interrupted status
      } 
    } 	 
  }

  public void shutdown() {
    done = true;
  }
}

Compliant Solution (AtomicBoolean)

In this compliant solution, the done flag is declared to be of type java.util.concurrent.atomic.AtomicBoolean. Atomic types also guarantee that writes are visible to other threads.

final class ControlledStop implements Runnable {
  private final AtomicBoolean done = new AtomicBoolean(false);
 
  @Override public void run() {
    while (!done.get()) {
      try {
        // ...
        Thread.currentThread().sleep(1000); // Do something
      } catch(InterruptedException ie) { 
        Thread.currentThread().interrupt(); // Reset interrupted status
      } 
    } 	 
  }

  public void shutdown() {
    done.set(true);
  }
}

Compliant Solution (synchronized)

This compliant solution uses the intrinsic lock of the Class object to ensure that updates are visible to other threads:

final class ControlledStop implements Runnable {
  private boolean done = false;
 
  @Override public void run() {
    while (!isDone()) {
      try {
        // ...
        Thread.currentThread().sleep(1000); // Do something
      } catch(InterruptedException ie) { 
        Thread.currentThread().interrupt(); // Reset interrupted status
      } 
    } 	 
  }

  public synchronized boolean isDone() {
    return done;
  }

  public synchronized void shutdown() {
    done = true;
  }
}

Although this compliant solution is acceptable, intrinsic locks cause threads to block and may introduce contention. On the other hand, volatile-qualified shared variables do not block. Excessive synchronization can also make the program prone to deadlock.

Synchronization is a more secure alternative in situations where the volatile keyword or a java.util.concurrent.atomic.Atomic* field is inappropriate, such as when a variable's new value depends on its current value (see VNA02-J. Ensure that compound operations on shared variables are atomic for more information).

Compliance with LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code can reduce the likelihood of misuse by ensuring that untrusted callers cannot access the lock object.

Exceptions

VNA00-J-EX0: Class objects are created by the virtual machine; their initialization always precedes any subsequent use. Consequently, cross-thread visibility of Class objects is already assured by default.

Risk Assessment

Failing to ensure the visibility of a shared primitive variable may result in a thread observing a stale value of the variable.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

VNA00-J

Medium

Probable

Medium

P8

L2

Automated Detection

Some static analysis tools are capable of detecting violations of this rule.

ToolVersionCheckerDescription
CodeSonar
8.1p0

JAVA.CONCURRENCY.LOCK.ICS
JAVA.CONCURRENCY.SYNC.MSS
JAVA.CONCURRENCY.LOCK.STATIC
JAVA.CONCURRENCY.UG.FIELD
JAVA.CONCURRENCY.UG.PARAM
JAVA.CONCURRENCY.VOLATILE

Impossible Client Side Locking (Java)
Missing synchronized Statement (Java)
Synchronization on static (Java)
Unguarded Field (Java)
Unguarded Parameter (Java)
Useless volatile Modifier (Java)

Eclipse4.2.0
Not Implemented
FindBugs2.0.1
Not Implemented
Parasoft Jtest
2023.1
CERT.VNA00.LORD
CERT.VNA00.MRAV
Ensure that nested locks are ordered correctly
Access related Atomic variables in a synchronized block
PMD5.0.0
Not Implemented
Fortify

Not Implemented
Coverity7.5SERVLET_ATOMICITYImplemented
ThreadSafe
1.3

CCE_SL_INCONSISTENT
CCE_CC_CALLBACK_ACCESS
CCE_SL_MIXED
CCE_SL_INCONSISTENT_COL
CCE_SL_MIXED_COL
CCE_CC_UNSAFE_CONTENT
CCE_FF_VOLATILE

Implemented

Related Guidelines

MITRE CWE

CWE-413, Improper Resource Locking
CWE-567, Unsynchronized Access to Shared Data in a Multithreaded Context
CWE-667, Improper Locking

Bibliography

[Bloch 2008]

Item 66, "Synchronize Access to Shared Mutable Data"

[Goetz 2006]

Section 3.4.2, "Example: Using Volatile to Publish Immutable Objects"

[JLS 2015]

Chapter 17, "Threads and Locks"
§17.4.3, "Programs and Program Order"
§17.4.5, "Happens-before Order"
§17.4.8, "Executions and Causality Requirements"

[JPL 2006]

Section 14.10.3, "The Happens-Before Relationship"



60 Comments

  1. One of the more painful lessons I learned wrt synchronization was that guaranteeing safe read/write of an object does not guarantee safe read/read of its fields. IME you have to guarantee thread-safety of every element, practically down to the byte level. (You can do this for objects only iff you can prevent unrestricted access to its fields.)

    I note that the NCCE/CS's are about volatile booleans. So this rule leaves un-addressed my painful lesson. We should either extend it to address safe read/write of Objects. Or, probably safer, restrict it to primitive data types.

    1. Good point. It doesn't address objects yet. The guideline LCK10-J. Do not use incorrect forms of the double-checked locking idiom has a good example for the use of volatile in this context.

      1. We discussed this earlier, but I'm making it official: I think volatile objects should be addressed in VNA06-J. Do not assume that declaring an object reference volatile guarantees visibility of its members.

        1. I wonder if the code example in the 'happens before' section should be a NCCE (with a CS following). I like the current text so the only reason to modify it is for uniformity, since we use NCCE/CS for our other rules.

          At this point CON11-C addresses volatile objects (as well as arrays), and addresses my painful lesson. So I don't think this rule needs any major modification. Perhaps a link to CON-11 to address volatile objects.

          1. Regarding your first question, I think we cannot detect that as a violation because it is allowed by the spec. There is nothing wrong with that per se.

            I suspect all valid uses of declaring an object volatile still belong to this guideline. CON11J deals with inappropriate uses of volatile.

            1. I'll agree with your 2nd point modulo s/object/primitive field/g

              1. Given that CON11-J is about volatile objects, and this rule seems to be only about volatile primitive types (eg not objects), perhaps we should change the title to reflect this?

                CON00-J. Declare shared primitive type variables as volatile to ensure visibility and prevent reordering of statements

                1. The last two NCE/CS deal with volatile objects. Consider a mutable class which has immutable members. That is, you can assign new immutable values to the members of the mutable class, say, using a setter. In such cases, you can declare the field volatile.

                  1. OK, I think I like the last two NCCE/CS pairs, and they belong in CON00-J, not CON11-J. Comments:

                    • Need nicknames for the NCCEs/CSs
                    • For stylistic consistency, the immutablePoint CS should leave out the methods, since they are identical to the methods defined in the NCCE.
                    • At least one of the occurrences of the word 'immutable' should hyperlink to its definition
                    • The map NCCE/CS does illustrate the point. The NCCE also violates CON26-J, as a partially-constructed map could become visible. You could fix this by using a safer object like ConcurrentHashMap. I think it would also be acceptable to just say "this rule violates CON26-J because..." and leave the NCCE as is. The CS does not violate CON26-J.
                    • Finally, on the map NCCE, you might add "If the Container allowed the map to be changed after initialization, you should see CON11-J for how to do this properly."
                    1. I don't believe sequential consistency is a requirement for implementing synchronization correctly. If you just look at your definition you will see that it is a very strong but "overly restrictive" memory model. Overly restrictive suggests that something less would do, which again means that sequential consistency is not a requirement.

                      Perhaps the requirement is that:

                      each read r of a variable v sees the value written by the write w to v such that:

                      • w comes before r in the execution order, and
                      • there is no other write

                      It may be that the best solution is to eliminate either the description of a solution from the introductory text to this rule or reference an overall description of this problem in some other text and just provide a minimal description of this particular guideline.

                      1. Synchronizing the code automatically guarantees sequential consistency and the developer won't have to reason about it. This concept is important if someone wants to prevent the reordering of statements, a common use case for volatile. It is not required to be sequential consistent all the time but depending on what you are doing you may need it. This guideline uses NCEs and CSs (2nd one in particular) that demonstrate this. Should I point this out in the text?

                        1. Synchronizing the code does not guarantee sequential consistency as you have defined it (and as defined by the JLS) which is a much stronger guarantee.

                          1. The JLS says:

                            If a program has no data races, then all executions of the program will appear to be sequentially consistent.

                            If you use synchronization (correctly), then there are no data races. Again, from the JLS:

                            This is an extremely strong guarantee for programmers. Programmers do not need to reason about reorderings to determine that their code contains data races. Therefore they do not need to reason about reorderings when determining whether their code is correctly synchronized. Once the determination that the code is correctly synchronized is made, the programmer does not need to worry that reorderings will affect his or her code.

                            A program must be correctly synchronized to avoid the kinds of counterintuitive behaviors that can be observed when code is reordered. The use of correct synchronization does not ensure that the overall behavior of a program is correct. However, its use does allow a programmer to reason about the possible behaviors of a program in a simple way; the behavior of a correctly synchronized program is much less dependent on possible reorderings. Without correct synchronization, very strange, confusing and counterintuitive behaviors are possible.

                            What am I missing?

                            Note that the JLS also says:

                            Sequential consistency and/or freedom from data races still allows errors arising from groups of operations that need to be perceived atomically and are not.

                            So sometimes we need even stronger guarantees than those provided by sequential consistency. This is the subject of the next few guidelines. (in fact, synchronization comes to the rescue again to provide stronger guarantees about atomicity)

                            1. OK, so this is my understanding. The JLS says:

                              Sequential consistency is a very strong guarantee that is made about visibility
                              and ordering in an execution of a program. Within a sequentially consistent execution,
                              there is a total order over all individual actions (such as reads and writes)
                              which is consistent with the order of the program, and each individual action is
                              atomic and is immediately visible to every thread.

                              The discussion bar says:

                              If we were to use sequential consistency as our memory model, many of the compiler and
                              processor optimizations that we have discussed would be illegal.

                              My understanding is that 1) sequential consistency prevents any reorderings and 2) Java does not implement a sequential consistency model; this is only being described for pedagogical purposes.

                              I agree that sequential consistency eliminates all reorderings, but eliminating all reorderings is not a requirement. The requirement is (I think):

                              each read r of a variable v sees the value written by the write w to v such that:

                              • w comes before r in the execution order, and
                              • there is no other write w' such that w comes before w' and w' comes before r in
                                the execution order.

                              I think this is the only problem which is solved by this guideline which recommends that programmers "Declare shared variables as volatile to ensure visibility and prevent reordering of statements"

                              1. My understanding is that the Java memory model guarantees the sequential consistency property for correctly synchronized programs. The discussion bar talks about strict use of sequential consistency as the memory model. It seems to imply that this is not required always so other concurrency primitives are also provided.

                                but eliminating all reorderings is not a requirement

                                The semantics of volatile were extended to prevent reorderings. That's why this guideline talks about sequential consistency. The second NCE/CS show this use-case.

                                I notice that the requirement that you have specified is not for sequential consistency and is more lax.

  2. Are we sure the BankOperation CS/NCE is correct? This is the opposite for C (see POS03-C. Do not use volatile as a synchronization primitive) and it really doesn't make any sense to me that it would work for Java. In C, volatile only prevents reorderings with other references to the same volatile variable, which would not prevent the set from being reordered before code which does not reference that volatile variable. I would be very surprised if this behavior were different in Java and consequently I think it is likely that this example is wrong. Is there a credible source for this solution? If not, I think we should remove this (or better still, use it as an example of something not to do in a different rule.

    1. Your point is valid and was a concern during the shaping of the "new" Java Memory Model. Under the new model, the lacking semantics of volatile (as you describe for C) were revised to ensure that any statements are not reordered. I think the JLS was also updated after this change. Off the top of my head, Brian Goetz, puts the story succinctly in this article New guarantees for volatile. Here's an opening quote from JSR-133:

      The semantics of volatile variables have been strengthened to have acquire and release semantics.
      In the original specification, accesses to volatile and non-volatile variables could be
      freely ordered.

      As an example of the particular NCE/CS, until one of the recent releases of the JDK, the security of the ClassLoader class depended on checking a volatile flag.

      1. The Java™ Language Specification Third Edition says that "Another change in this edition is a complete revision of the Java memory model, undertaken by JSR-133."

        1. This means that the JLS 3e conforms to JSR-133, right? But IIRC, it doesn't describe the extended semantics for volatile (or it is hidden somewhere, where I can't find it). Are you recommending that we prefer JSR-133 over JLS 3e?

  3. Volatile accesses do not guarantee the atomicity of composite operations such as incrementing a variable. In i++ for example, i is read, incremented, and then written. It is possible that another thread would write to i after it has been read but before it has been written.

    1. Very true. That's why they have a limited set of use-cases which is the focus of this guideline. This is the reason why we have VNA02-J. Ensure that compound operations on shared variables are atomic.

  4. I noticed that the following lines were deleted.

    This may not be true in the happens-before order because a future read can always see the default or previous value of v instead of the one set in the most recent write. This guarantee is provided by the sequential consistency property of volatile accesses.

    I think this line is essential to understand why the happens before relationship is not enough for volatile (even if reiterates to some extent the guarantees of sequential consistency). Otherwise, people would generally assume that happens-before is safe enough.

    IMHO, the quote shown below is useful when it comes to deciding when to prefer volatile over synchronization. The argument as we've heard before is - always synchronize your code and don't bother about using volatile. However, this is not always the best solution, for example, as the same quote states:

    Because the guarantees of code present before the volatile write are weaker than sequentially consistent code, volatile as a synchronization primitive, performs better. Because no locking is involved, declaring fields as volatile is likely to be cheaper than using synchronization, or at least no more expensive. However, if volatile fields are accessed frequently inside methods, their use is likely to lead to slower performance than would locking the entire methods. [Lea 00].

    Volatile is also more useful when there are more reads as compared to writes.

    Finally, in the sentence:

    However, this does not mean that statements 1 and 2 are executed in the order in which they appear in the program. They may be freely reordered by the compiler. In fact, if statement 1 constituted a read of some variable x, it could see the value of a future write to x in statement 2. These stronger volatile semantics, however, increases the cost of volatile almost to cost of synchronization.

    The last line is a bit misplaced. The first few lines are not about strong guarantees of volatile but weak as they describe what cannot prevent reordering of all statements. In fact, I would argue that the performance is better than synchronization for these reasons. The only thing that can increase the cost of volatile accesses is when you are making too many calls to fetch the results of the volatile variableas Doug Lea states in the quote that was deleted.

    1. Maybe we should meet to talk about this Monday afternoon after class. The other thing you could try is to rewrite this without using the term "sequential consistency" which is not guaranteed by any property of Java other than that if you develop a "correctly synchronized" program it appears to be sequentially consistent.

      A correctly synchronized program is one that has no data races. The way that you eliminate data races is that you make sure that happens-before semantics upon which the program depends are guaranteed by the Java Memory Model.

      1. I cede that happens before relationships provide enough guarantees in case of volatile variables and do not need any references to a weaker "sequential consistency of accesses". The ambiguity is the difference between happens-before consistency and happens-before relationship. These are not interchangeable terms. Volatile writes happen before the reads. Therefore, this prevents them from being happens-before consistent. If they are not happens-before consistent, they would not exhibit the unaccetable behaviors defined in Trace 17.5 of the JLS. So my previous comment's first quote's last line should read happens-before relationship instead of seq consistency.

        Another close look at the JLS suggests that my most recent interpretation described above is wrong after all.

        A set of actions A is happens-before consistent if for all reads r in A, it is not the case that either hb(r, W(r)), where W(r) is the write action seen by r or that there exists a write w in A such that w.v = r.v and hb(W(r), w) and hb(w, r).

        With volatile, hb(r, W(r)) is not true.

  5. I'm fixing a couple of problems.

    First, this statement about the diagram is incorrect:

    For example, if statement 1 constituted a read of some variable x, it could see the value of a future write to x in statement 2.

    This reordering cannot take place because the compiler has to maintain the intra-thread ordering. An incorrect reordering can only occur if x is a shared variable and it is modified by one thread and read by another without correct synchronization. For example,

    The second problem is this sentence:

    These stronger volatile semantics, however, increases the cost of volatile almost to cost of synchronization, for example if thread 2 were to perform a write before the volatile read.

    While this is true, it compares the new volatile semantics against the old volatile semantics, none of which is obvious from the context.

    1. An incorrect reordering can only occur if x is a shared variable and it is modified by one thread and read by another without correct synchronization. For example,

      Yes, I think it is important to state that all statements use shared variables.

      While this is true, it compares the new volatile semantics against the old volatile semantics, none of which is obvious from the context.

      I pointed this out in my previous comment. This line occurred after the text about when to prefer volatile and when synchronization. Now it simply doesn't fit.

  6. A minor editing point... the keyword volatile should be in code-font, but preferably you can talk about the concept of "volatile" using the English language word which does not require formatting.

    • The 'non-volatile guard' NCCE violates CON26-J, it is about a partially-constructed object. So does the 'partial initialization' NCCE.
    • The ImmutablePoint class needs to be final. If it isn't final, an attacker can subclass an 'EvilPoint' class, which is mutable, then call setPoint( x), where x is an EvilPoint. They then proceed to wreak concurrency havoc by changing x.
    • The ImmutablePoint NCCE/CS is correct, but not that useful, as you can't reliably prevent TOCTOU race conditions w/o additional locking. Prob it just needs a disclaimer to that effect.

    SureLogic recommend that volatile is only good iff:

    • the variable is only written once (it is never written a 2nd time, otherwise the 2nd write and reads constitute TOCTOU race conditions)
    • The variable's write does not depend on its current value. (again due to TOCTOU race conditions).

    This is very strict; it invalidates our ImmutablePoint NCCE/CS pair. As I said above, the ImmutablePoint is OK as long as you don't care about TOCTOU race conditions. FTM the ImmutablePoint CS may violate CON09-J.

    Given those principles, I'm not sure a 'use volatile to solve problems' rule is warranted. Perhaps this should be a "when is volatile useful / useless' rule.

      • Yes. We could state that if you like. NCEs are allowed to violate multiple rules.
      • True. I note that our definition of immutability does not cover this important case but it should.
      • Yes, in the absence of synchronization (of the setter) its use is limited. I think we can add a disclaimer which refers to the new CON09. (this is CON09 anyway, just weaker)

      Regarding goodness of volatile:

      • I can't see how the NCE/Compliant Solution (visibility) violates CON09. I would claim that it is a subset of CON09. If there were multiple statements in the setter then obviously it would. In that case, synchronizing the setter is a better option. I maintain that the getter need not be synchronized.

      I think the title is getting misleading. This guideline is not touting the use of volatile to solve all problems but suggesting the circumstances under which it can be used safely. Other guidelines do a good enough job to tell you when not to use it.

      Regarding the two requirements that you say are very strict:

      • If a variable is written only once, it is pretty much like declaring it final. How else would you ensure that it is written to only once? If you can, sure. This doesn't take into account a setter which is synchronized and a getter which is not but the variable is declared volatile. This is a valid use.
      • True. Subject of CON01. But by the same argument, if there are multiple reads and very few writes, the performance of using volatile will be much better than synchronizing all methods, including the getters that do no nonatomic operations. Excessive synchronization also leads to deadlocks.
      1. I've finished editing the rule. I got rid of the 4th NCCE/CS b/c it is so similar to CON26-J; it doesn't seem worthwhile to repeat it. Also I'm not convinced the CS is really valid (it has IMO the same fallacy as the cheap read/write lock trick from CON11-J.)

        1. A lot of these concurrency issues depend on the context in which you are using them. The choice of what to use and what not to depends largely on the context. The 4th NCE/CS detailed when you can actually use volatile safely. If one removes it, it creates more ambiguity for the developer. Imagine if I ask - should I declare fields holding references to mutable objects as volatile? There is no black and white answer. It may/may not be safe. My idea was to show the cases in which it is safe (covered by the 4th NCE) and where it is not (subject of CON11 and other guidelines). That said, I stated in the compliant solution that this approach has limited uses and better alternatives are available. This rule now does not capture the use of volatile for "safe one time publication" as suggested by [Goetz 07].

      2. Maybe we should get together tomorrow AM and discuss this further. I have some fundamental issues with alot of things being said here. Here is a sampling:

        1. "This is a guidelines about how to use volatile".

        We don't have guidelines that describe how to use language features; that is what a language book is for. Guidelines are enumerations of errors. They basically say "don't do the following stupid thing". The compliant solution says "if you were trying to do this, this is how you do it".

        At a minimum, a developer should be able to look at a guideline and determine if they made that mistake. Better still, an analyzer should be able to look and determine if that mistake was made. If we end up with a guideline that says "Know when to use volatile" we might as well just delete it now.

        2. IMHO, the use of volatile is fine for concurrent access to variables where the concurrent access consists of a single atomic read or write. It falls short when multiple operations need to be performed in an atomic fashion. I am quite happy to show volatile as a compliant solution in any situation in which it applies.

        3. It is quite possible that we don't want to have a guideline that says "use volatile to do something", especially when other solutions are possible. Instead we may need to reorganize this section (again) so that the guidelines describe problems, for example, "Don't guard a block a code with a shared variable that can be reordered". This describes the problem and allows for multiple solutions without necessary preferring one to another (the relative advantages and disadvantages of the solutions should be explained).

        1. Agreed, let's meet & discuss.

          I'm going to point out a counterexample to (1) from the C wiki: ARR00-C. Understand how arrays work

          Also the info in this rule is good and should not be dropped. But I'm open to reorganizing the info, and perhaps getting rid of this rule.

          1. At one point, the C standard had some introductory sections that provided generic background information for the chapter. We only ended up with a few of them (like this one) so we incorporated them into these informational "Understand" guidelines.

            After the concurrency meeting last week, I accepted the suggestion that we have add introductory sections to each section to provide the required/useful background that wasn't in the form of a guideline (although we may not all be on the same page here yet). I added a section like this for concurrency, so that the guidelines could all be guidelines.

        2. Agreed. I also suggest going through Goetz 07.

          In hindsight, CON00 was about using volatile and synchronization and other approaches to solve a common problem. Now we have more explanations and unfortunately, some of them seem to suggest that we are recommending the use of volatile over synchronization. I should also say that synchronization is not the panacea. Excessive synchronization can lead to deadlocks.

          I still prefer the old title i.e. ensure visibility and prevent reordering because volatile has very valid uses (such as a volatile status flag). We we get rid of this rule, I am not sure where the valid uses cases will go. Note that this rule is referenced by other guidelines in different sections too because it shows how to use status flags and so on.

  7. As discussed, now the text from this guideline has been moved to the introduction. The old one has been split into two, status flags and volatile guards.

    1. My comments on CON28-J apply here as well...any rule that mentions 'volatile' in the solution title is wrong.

      The NCCE is about the fact that no locking is done to guarantee visibility of the done variable, and volatile is a good solution, but not the only one. Should at least have a CS that uses synchronized methods. Maybe one that uses a Lock object, too.

      Ideally this NCCE should go in a rule that says "use locks to guarantee each thread sees the latest state of shared data" (eg the rule is all about visibility.) I don't see such a rule in this section...perhaps CON00-J should be revamped to be this rule.

      1. I think there is a strong argument for declaring status flags volatile instead of running away and synchronizing all methods to ensure visibility of just a single flag. Synchronization is overkill in this case because:

        1. Excessive synchronization leads to deadlocks (a violation of several rules)
        2. There are issues wrt calling alien methods from synchronized regions (a violation of CON08-J. Do not call alien methods that synchronize on the same objects as any callers in the execution chain)
        3. A time-consuming method should not be synchronized (a violation of VOID CON06-J. Do not defer a thread that is holding a lock). And the locking CS would violate it as in this example, we are deferring the thread.

      2. Synchronization can be listed as an exception to this guideline. If there is need to synchronize all methods then it may be possible to synchronize the constructor using block synchronization and an internal private final lock object. If there is no need for any synchronization it may be better to have volatile status flags and leave the title like this. Would this satisfy your comment?

        PS: By the same argument, if we cannot suggest volatile, the word "lock" should also not occur in the title of any guideline because it is not fool-proof and the best choice always.

        1. I'm thinking a good title might be something like:

          "Ensure visibility of atomically modified shared variables"

          This sets it off a bit from:

          VNA01-J. Ensure visibility of shared references to immutable objects

          1. I've changed the title and put the criteria for what constitutes an atomic operation.

            1. After renaming, it strikes me that long and double are not guaranteed by the spec to be accessed atomically. But we still need to declare them volatile and ensure their visibility.

  8. I like this title much better. But the intro needs some work:

    • Needs a link to CON25-J (about 64-bit values)
    • Needs to say "if a shared var has the folliwng 3 characteristis..."

    Volatile is a good solution, but is not the only good solution. Prob should add a CS using synchronized, or explicit locking to achieve the same effect. (You can mention that the volatile CS has the following advantages and disadvantages...)

      • How about this new title - "Ensure visibility of shared primitive variables on accesses"

      The old title almost says that long and double do not apply to this guideline because they are permitted to be not accessed atomically. But we just care about accesses, after all.

      • I've added a link to CON25.
      • I am not sure about saying "the variable has the characteristics" because these are characteristics of the accesses. I am not quite sure what you meant there - a shared variables cannot have these characteristics, right?

      If I add synchronization as a CS it would violate the 3 guidelines I stated in my previous comment. Shall we add that as an exception instead? The conditions in the introduction also states: "There is no need to synchronize the code to access the variable". This can be done using your getter approach, though it should say that it is more expensive.

      This guideline still recommends volatile over other synchronization primitives for this NCE/CS but there appears to be good reason to do so.

  9. Here are my main comments:

    • You may want to defer the discussion about atomicity because the title does not mention it. It can be mentioned towards the end of the introduction. If you are ensuring visibility, in any case, the atomicity aspect is covered without the need of anyone referring to CON25, which essentially suggests the solutions recommended in this guideline.
    • I am not sure why you use "If a shared primitive variable has these characteristics". We are talking about operations/accesses in the criteria. To avoid ambiguity, it can be written like "The following criteria allows a variable to be declared volatile:". I also suggest putting this in the first CS instead of the introduction. The introduction can only mention the visibility issue then.
    • In the Compliant Solution (synchronized), the text needs to be changed a bit (I'll take care of that). Also, bool should be boolean.
  10. I am not sure about this extra condition for volatile:

    "Only a single thread ever updates the value."

    If a single thread updates the value nonatomically, but others are allowed to read, problems may arise.

    1. Not sure what this is about. If a thread updates a value nonatomically, and other threads can see the 'intermediate value', that is a problem, but it is best left to another rule, prob CON11-J.

      It's always stated that a happens-before occurs between any write and subsequent read of a variable. I've never heard this stated, but you prob also need a happens-before between any write and subsequent write as well. If only one thread writes the variable, you get that HB for free. If two distinct threads can write to the var, I don't think you get the HB, so you have to use some other locking mechanism. Which would prob render the volatile unnecessary.

      1. I meant, if only one thread is writing to a shared variable:

        volatile int var;

        1. Thread 1 does var++;
        2. While 1. is executing Thread 2 comes and reads var

        Thread 2 sees a stale value in this case right? Basically, the use of volatile is not recommended for composite operations even when one thread is updating the value while others are reading it (CON01). So I am not sure if the condition "Only a single thread ever updates the value." needs to appear in the criteria for using volatile (in this guideline).

        It could use something like : "Only a single thread ever updates the value atomically" but then this is what the fourth condition usually means.

        1. I'd say the condition is necessary but not sufficient. As such, it can stay in as is. If you want the four conditions to be sufficient, that's another matter, but that's why this rule exists.

          BTW doing i++ wiolates the condition that the new value relies on the old value.

          1. Hmm, I realize that i++ violates condition 2. OTOH, if conditions 1,2 and 4 are true there doesn't appear to be any reason to have condition 3. Why not allow multiple threads to update the value then?

            1. It's always stated that a happens-before occurs between any write and subsequent read of a variable. I've never heard this stated, but you prob also need a happens-before between any write and subsequent write as well. If only one thread writes the variable, you get that HB for free. If two distinct threads can write to the var, I don't think you get the HB, so you have to use some other locking mechanism. Which would prob render the volatile unnecessary.

              1. It appears [Goetz 06] uses this:

                Writes to the variable do not depend on its current value, or you can ensure that only a single thread ever updates the value;

                in the same sentence. So it could mean - if you can ensure that a single thread updates the value, and no other conditions are violated (especially the locking one), then the write to the variable may even depend on its current value.

                I think I am just slightly unsure about the security of approaches that rely on "ensuring only one thread updates the value".

  11. I suspect class objects by themselves are visible, so EX1 should apply to class objects in general, not just arrays of class objects.

    1. Agreed. You can point this out if you like. Thanks.

  12. The JMM Mailing List link (https://mailman.cs.umd.edu/mailman/private/javamemorymodel-discussion/2007-August/000083.html) requires a username and password, so I don't think it should be cross-referenced in this guideline.

  13. I'm reluctant to change this guideline, as it's already been tech reported. That said, surely we ought to call this "safe publication" rather than "visibility"... right? That's the term that you'd want to use in a web search to find the right information.

    1. Depends. We have other rules on safe publication, and the techniques for safe publication of an object are different than those for primitive data types.

      1. The techniques are indeed different. But the goal in both cases remains the same: ensuring that the relevant data has been safely published to other threads that need visibility.

        Overall, I remain reluctant to change the guideline. But consistency would be nice.

  14. Coverity's GUARDED_BY_VIOLATION checker flags a variable if it is protected by many synchronized blocks, but it is accessed unprotected once. (Coverity assumes the dev just forgot to put a synch block around that one access).

    Currently this checker is mapped to this rule, but I wonder if we have a better rule for it? Possibly VNA01-J. Ensure visibility of shared references to immutable objects is also a good candidate? OTOH both guidelines are misleading b/c they focus primarily on visibility.

    Suggestions?