Skip to end of metadata
Go to start of metadata

Compound operations are operations that consist of more than one discrete operation. Expressions that include postfix or prefix increment (++), postfix or prefix decrement (--), or compound assignment operators always result in compound operations. Compound assignment expressions use operators such as *=, /=, %=, +=, -=, <<=, >>=, >>>=, ^= and |= [JLS 2015]. Compound operations on shared variables must be performed atomically to prevent data races and race conditions.

For information about the atomicity of a grouping of calls to independently atomic methods that belong to thread-safe classes, see VNA03-J. Do not assume that a group of calls to independently atomic methods is atomic.

The Java Language Specification also permits reads and writes of 64-bit values to be non-atomic (see rule VNA05-J. Ensure atomicity when reading and writing 64-bit values).

Noncompliant Code Example (Logical Negation)

This noncompliant code example declares a shared boolean flag variable and provides a toggle() method that negates the current value of flag:

Execution of this code may result in a data race because the value of flag is read, negated, and written back.

Consider, for example, two threads that call toggle(). The expected effect of toggling flag twice is that it is restored to its original value. However, the following scenario leaves flag in the incorrect state:








Reads the current value of flag, true, into a temporary variable




Reads the current value of flag, (still) true, into a temporary variable




Toggles the temporary variable to false




Toggles the temporary variable to false




Writes the temporary variable's value to flag




Writes the temporary variable's value to flag

As a result, the effect of the call by t2 is not reflected in flag; the program behaves as if toggle() was called only once, not twice.

Noncompliant Code Example (Bitwise Negation)

The toggle() method may also use the compound assignment operator ^= to negate the current value of flag:

This code is also not thread-safe. A data race exists because ^= is a non-atomic compound operation.

Noncompliant Code Example (Volatile)

Declaring flag volatile also fails to solve the problem:

This code remains unsuitable for multithreaded use because declaring a variable volatile fails to guarantee the atomicity of compound operations on the variable.

Compliant Solution (Synchronization)

This compliant solution declares both the toggle() and getFlag() methods as synchronized:

This solution guards reads and writes to the flag field with a lock on the instance, that is, this. Furthermore, synchronization ensures that changes are visible to all threads. Now, only two execution orders are possible, one of which is shown in the following scenario:








Reads the current value of flag, true, into a temporary variable




Toggles the temporary variable to false




Writes the temporary variable's value to flag




Reads the current value of flag, false, into a temporary variable




Toggles the temporary variable to true




Writes the temporary variable's value to flag

The second execution order involves the same operations, but t2 starts and finishes before t1.
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.

Compliant Solution (Volatile-Read, Synchronized-Write)

In this compliant solution, the getFlag() method is not synchronized, and flag is declared as volatile. This solution is compliant because the read of flag in the getFlag() method is an atomic operation and the volatile qualification assures visibility. The toggle() method still requires synchronization because it performs a non-atomic operation.

This approach must not be used for getter methods that perform any additional operations other than returning the value of a volatile field without use of synchronization. Unless read performance is critical, this technique may lack significant advantages over synchronization [Goetz 2006].

Compliant Solution (Read-Write Lock)

This compliant solution uses a read-write lock to ensure atomicity and visibility:

Read-write locks allow shared state to be accessed by multiple readers or a single writer but never both. According to Goetz [Goetz 2006]:

In practice, read-write locks can improve performance for frequently accessed read-mostly data structures on multiprocessor systems; under other conditions they perform slightly worse than exclusive locks due to their greater complexity.

Profiling the application can determine the suitability of read-write locks.

Compliant Solution (AtomicBoolean)

This compliant solution declares flag to be of type AtomicBoolean:

The flag variable is updated using the compareAndSet() method of the AtomicBoolean class. All updates are visible to other threads.

Noncompliant Code Example (Addition of Primitives)

In this noncompliant code example, multiple threads can invoke the setValues() method to set the a and b fields. Because this class fails to test for integer overflow, users of the Adder class must ensure that the arguments to the setValues() method can be added without overflow (see NUM00-J. Detect or prevent integer overflow for more information).

The getSum() method contains a race condition. For example, when a and b currently have the values 0 and Integer.MAX_VALUE, respectively, and one thread calls getSum() while another calls setValues(Integer.MAX_VALUE, 0), the getSum() method might return either 0 or Integer.MAX_VALUE, or it might overflow. Overflow will occur when the first thread reads a and b after the second thread has set the value of a to Integer.MAX_VALUE but before it has set the value of b to 0.

Note that declaring the variables as volatile fails to resolve the issue because these compound operations involve reads and writes of multiple variables.

Noncompliant Code Example (Addition of Atomic Integers)

In this noncompliant code example, a and b are replaced with atomic integers:

The simple replacement of the two int fields with atomic integers fails to eliminate the race condition because the compound operation a.get() + b.get() is still non-atomic.

Compliant Solution (Addition)

This compliant solution synchronizes the setValues() and getSum() methods to ensure atomicity:

The operations within the synchronized methods are now atomic with respect to other synchronized methods that lock on that object's monitor (that is, its intrinsic lock). It is now possible, for example, to add overflow checking to the synchronized getSum() method without introducing the possibility of a race condition.

Risk Assessment

When operations on shared variables are not atomic, unexpected results can be produced. For example, information can be disclosed inadvertently because one user can receive information about other users.




Remediation Cost









Automated Detection

Some available static analysis tools can detect the instances of non-atomic update of a concurrently shared value. The result of the update is determined by the interleaving of thread execution. These tools can detect the instances where thread-shared data is accessed without holding an appropriate lock, possibly causing a race condition.

Inconsistent synchronization
Field not guarded against concurrent access
Call to static Calendar
Call to static DateFormat
Static Calendar field
Static DateFormat


Parasoft Jtest9.5TRS.SSUG, TRS.MRAVImplemented




Related Guidelines


CWE-366, Race Condition within a Thread
CWE-413, Improper Resource Locking
CWE-567, Unsynchronized Access to Shared Data in a Multithreaded Context
CWE-667, Improper Locking




  1. Synchronization is not just used to ensure atomicity. It is valid to use it for ensuring visibility, as an alternative to volatile. In fact, most often it is the safer way because the programmer is not expected to reason about the memory model specifics. Sometimes, it can be tricky to reason about using volatile.

    My fear about emphasizing the discussion about visibility by bringing an xref to CON00 is that it may indicate that we are suggesting a possibly less secure alternative to synchronization. Or worse, suggesting synchronization to ensure atomicity and volatile to ensure visibility. It is hard to say "your shared variables need to also be visible; for more, see CON00-J" because the visibility condition is not required in addition to atomicity, it is guaranteed by synchronization, implicitly. The use of synchronization does not depend on CON00. The focus of this rule is on the correct use of synchronization and the concurrency utilities.

    I have renamed the guideline to lay more emphasis on atomicity. The visibility occurs second now and is more like a by-product.

      • The intro is good. I think I would split the paragraph into two, the second one discusses the concurrrent atomic classes. IOW the 1st paragraph describes the problem, the 2nd describes the solution.
      • Need nicknames on the NCCE's & CS's.
      • The 1st NCCE mentions that itemsInInventory can be accessed by multiple threads. If this is true, then the CS's don't fix the problem, since any thread can modify this field directly, instead of using removeItem(). The NCCE & CS's are only worthwhile if itemsInInventory is not directly modifiable by any thread, and they can only modify it via the removeItem() method. If this is so, the NCCE is still bad b/c multiple threads calling removeItem() can cause a race condition, but the CS's are good for the reasons each lists.
      • The 3rd CS is subject to a race condition. Suppose itemsInInventory == 1, and two threads perform the '>0' comparison simultaneously and both decide to decrement itemsInInventory. The synchronized block guarantees they will not decrement it at the same time. But when both are done, itemsInInventory == -1! Fix by moving the synchronized keyword outside the comparison. Or use the double-lock idiom.
      • The 2nd NCCE needs a sentence or two saying what kind of thread-safety issue might happen. "For instance, if two threads..."
      • I still think this should be merged with CON07-J, we can talk about it tomorrow.
        • They are now separate paragraphs.
        • Done.
        • Multiple threads can only use removeItem() to change the state of the object because it is declared private. The NCE is vulnerable to all kinds of race conditions as noted in the guideline.
        • Changed the scope of the block. I am tempted to use double checked locking but have omitted it for the sake of simplicity.
        • Added explanation.
        • ...
  2. Hmmm, I haven't looked at CON07 yet. At a quick glance, it is not apparent these guidelines are distinct. I seriously doubt that the difference between these guidelines will be if they apply to expressions versus statements. C language, at least, has expression-statements.

    1. CON07 looks at statements and also a few other things:
      1. Debunks the myth that a grouping of multiple calls to independently atomic methods of thread-safe classes is atomic (many people have this misconception that thread-safe classes are safe for use).
      2. Shows how to extend existing thread-safe classes in the API to add new functionality that is atomic.
      3. This guideline is about atomicity of operations on a shared primitive fields (useful when designing the API). CON07 discusses atomicity of operations on shared objects (useful when extending an existing API).
      4. The distinction might be useful for automatic analysis and to flag violations.

      However, the solution is essentially the same - correct synchronization.

  3. I modified the NCE for the Noncompliant Code Example (volatile). If you like this, I can try to propagate it to the other examples. I think this is slight more realistic and also hides eliminates / hides the error conditions by turning the problem into a handled condition.

    1. I think I like the change. You might want to eliminate the return statements and just set itemsInInventory directly because the return type is now void.

      I can change the other examples if you prefer. Do we need to warn about integer overflow somewhere, when using concurrent utilities?

    2. Yes, however we need some work to get this to compile (sad)

      The above code will not compile. Java doesn't have a way to implement underStocked, in particular the method must always throw an unchecked exception (as I did). The type system is not savvy enough to realize this is the case. The other problem is the use of > on the check against MAX_INVENTORY

      If MAX_INVENTORY is defined as I did as Integer.MAX_INTEGER this code can be compiled down to

      because nothing is larger than Integer.MAX_INTEGER (and Java has silent, might I say evil, wrapping of integer types).

      Here is one idea

      The above code uses exceptions and two new methods to check the state. Another approach is using assert to simplify the code (and foist the problem onto the user of this code).

      This is far more compact and might help to focus the reader onto the point of the item (which is not how to deal with a class invariant). The behaviour in this case is to throw an AssertionError if the assert does not evaluate to true (assuming, of course, that the VM has assertions enabled).


      1. Tim,

        Here are my thoughts:

        • You are right that the examples will not compile as we'd expect them to.
        • I was trying to avoid throwing an unchecked exception from underStocked() because the client could take corrective action such as request more supplies when inventory is empty. Or when it is full, it could take alternate actions such as create a new variable to store the surplus.
        • This means that we could use a checked exception(s) instead and I am fine with that other than the additional clutter.
        • Your class NoncomplientVolatile1 doesn't appear to be thread-safe because after the isEmpty() check and before itemsInInventory--, a thread could have removed some inventory. Similarly, between the check isFull() and itemsInInventory++ , some thread could have added inventory and consequently, itemsInInventory++ will overflow.
        1. On the third bullet – it is not thread-safe this is the "noncomplient volatile" example.

          I'm not a big fan of the use of checked exceptions in most cases (they should be used very rarely as they can result in terrible APIs). Look at java.util.LinkedList.getFirst() and getLast() or java.util.Queue.remove() for good API examples of this ilk.

          At issue is the clutter you introduce to the user of your API. (ignoring concurrency for a second).

          Being understocked is not exceptional so the API should have a way for the programmer to find out without an exception being thrown. And it does. But the use of the checked exception foists the try-catch block on every single use.

          I'm okay with the assertion idea (I think MET02-J is correct but it maybe should say more about where assertions are appropriate – they can help make systems fail-fast which is good)

            • Ah, I see that you were talking about changing the NCE. This is admissible, however, it is going to introduce two issues in a single noncompliant code example (check-then-act problem and the ++ operator problem). It might be easier/clearer to focus only on the ++ operator nonatomicity problem for this rule.
            • I wanted to avoid using either of checked or unchecked exceptions here because it would complicate the code examples. I agree with the fact that the inventory being empty does not imply an exceptional condition that should be forced on the client. An unchecked exception might also be unsuitable because we would ideally not want to halt the program when a user requests too many items. We can always ask the user again to request a smaller number of items or handle this condition in a different way. My first choice was a status code because of the simplicity. The current change uses underStock() which seems ok. This method stays in this class and handles increasing the inventory and if it is not successful, it may choose to throw a runtime exception. Using underStock() abstracts the nonrelevant details from this guideline.
            • I am not sure if the discussion about where assertions are appropriate belongs to MET02 because assertions should not be generally used for checking method arguments. Assertions are useful for other purposes but I am not sure if it can be added as a separate guideline (use of assert doesn't appear to exist in the C standard). If yes, then I can add it as a separate guideline.
            1. This is admissible, however, it is going to introduce two issues in a single noncompliant code example (check-then-act problem and the ++ operator problem). It might be easier/clearer to focus only on the ++ operator nonatomicity problem for this rule.

              The original example also has the parameter check, therefore, both issues come up. The parameter check has to be removed to just focus on the ++/-- operator problem in isolation. For example:

              Noncompliant Code Example (increment/decrement)

              Pre- and post-increment and pre- and post-decrement operations in Java are non-atomic in that the value written depends upon the value initially read from the operand. For example, x++ is non-atomic because it is a composite operation consisting of three discrete operations: reading the current value of x, adding one to it, and writing the new, incremented value back to x.

              This noncompliant code example contains a data race that may result in the itemsInInventory field missing the fact that callers returned or removed items.

              For example, if the removeItem() method is concurrently invoked by two threads, t1 and t2, the execution of these threads may be interleaved so that:








              reads the current value of itemsInInventory, 100, into a temporary variable on the thread's stack




              reads the current value of itemsInInventory, (still) 100, into a temporary variable on the thread's stack




              decrements the temporary variable to 99




              decrements the temporary variable to 99




              writes the temporary variable value to itemsInInventory




              writes the temporary variable value to itemsInInventory

              As a result, the effect of the call by t1 is not reflected in itemsInInventory. It is as if the call was never made. This "lost call" phenomenon can occur with concurrent calls to returnItem() or concurrent calls to removeItem() and returnItem().

              1. Yes that is correct. However, it seems if I remove the check then we have to also fix it in the compliant solutions and specifically talk about integer overflow in this guideline which may complicate it some more. Besides, the first NCE provides a gradual introduction so I doubt it shoule be eliminated.

                As I said, your example is good, however, I prefer abstracting the exception handling part to underStocked() and overStocked() methods. These methods can be called more than once when multiple threads find that inventory is low. However, this issue can be handled when these methods install checks to determine whether the inventory is at its minimum/maximum before atomically taking some action like filling up the inventory. Do you see any issues with this approach?

                1. Well, for the compliant solutions that use locking you have to consider those methods in the locking policy, i.e., the lock must be held when these methods are invoked.

                  In our tool, if we name the lock InventoryLock, we would annotate:

                  lock preconditions are hard to explain when you can't see the implementation (in this case that these methods examine the itemsInInventory field.

                  I figured out how to copy and paste the item into a comment – so I'll take a crack at it without editing the main item.

                  1. I see your point about annotations and the level of detail a tool might require. However in this case, I am not sure if any additional locking is necessary when you declare underStocked() and overStocked() as private methods. Note that we are already calling these methods from synchronized methods/blocks in the CSs. This information is not captured in the current NCEs/CSs but we could perhaps mention it in the text. Would this help?

                    Feel free to create new pages if you would like to demonstrate code samples - it might be easier to work with. Thanks!

              2. Tim, I like your table. 8^) However, how do you know that the current value of itemsInInventory, (still) 100, into a temporary variable on the thread's stack? I would think it would more likely that this value would be moved into a register and then incremented. Consequently for now I am just going to say "temporary variable" and not give a location.

        2. Noncompliant Code Example (increment/decrement)

          Here is another proposal for the top example based Dhruv's comments. I think this is an improvement because the item can talk about the calls to isEmpty() and isFull() to do checking and their impact on safety and visibility.

          I'll do a set of edits if Dhruv thinks this looks okay as a starting point.

  4. We could use IllegalStateException if we have decided that there is nothing that can be done to handle the condition (like request a method to add more inventory).

    If you were to eliminate the for loop, the design might be a bit questionable because another thread may have updated the inventory in the meantime (this is a producer consumer like problem). But it seems now you have eliminated the returnItem() method making this point moot for the purpose of this example (but not for a real-world scenario). (the for loop is still necessary because two thread may invoke removeItem())

    The previous example abstracted the exception handing part by calling the private method underStocked() from the else condition. This method could request more inventory and choose to throw an IllegalStateException if it failed. Not a bad approach - though it would not compile without providing the implementation of underStocked().

    1. Eventually I rewound all the way to our earliest examples (see the current version above).

      I think the problem with pushing off to the understocked() method to request more inventory is that the removeItem() method still needs to take an item out of inventory (or not) and provide an indication of what happened. We really can't allow the method to go past the minimums, so this means "not" meaning that we still need to test for the condition and return an error. Consequently this approach is more complicated. I think the above is as simple an example we can get. The only other thing I can think to do is to have an example of code that generates a "unique id" by always incrementing a counter and ignoring overflow. However, I think this is unnecessary because we don't need to show the entire solution space, we just want to enumerate the entire error space.

      The main thing I'm wondering is if we shouldn't invert the condition for the test against min? I think this allows us to pull the throw out of the synchronized block in some situations. I'm not yet sure I understand or trust the dynamics between exception handling and thread safety in Java, but perhaps you guys understand this better.

      1. I don't think pulling the the test outside the synchronized region is safe. For example, two threads will find that the inventory is empty and both will try to decrease the inventory count then. If however, the region is synchronized, two threads will not be able to carry out the test together at the same time. If you declare itemsInInventory as volatile, there is still the problem that after the test and before the decrement, another thread could have altered the value of itemsInInventory so that it would overflow on the current execution.

        1. No, here is what I meant:

          This should synchronize less code, because the creation of the IllegalStateException object takes place outside the synchronized block.

          1. While this reduces the amount of code in the synchronized block, it comes at a price:

            Even now, if you release the lock in the finally block, another thread might be free to increase the inventory before the exception is thrown (I note that we eliminated returnItem(), but I personally think that we shouldn't trade security for a little performance gain. Most code I've seen throws the exception first depending on the test because then we don't have to perform all the regular operations in a long if block).

            Technically, the exception should not be thrown whilst the method can still retry the operation of removing the item when another thread could have increased the inventory since the test. Just a thought.

            PS: I am going to remove the synchronized keyword from this CS because it uses Reentrant locks.

            1. > another thread might be free to increase the inventory before the exception is thrown

              yeah, sure. who cares? we are trying to make sure that operations on itemsInInventory are atomic. this code guarantees that they are, and that either the value is decremented or an exception is thrown. i don't see any loss of security.

              i really don't see retrying an operation like this where you are waiting for some rare event to occur. just return an error and let the application / user decide if it wants to retry.

              ok on removing the keyword.

        2. No, here is what I meant:

          This should synchronize less code, because the creation of the IllegalStateException object takes place outside the synchronized block.

          1. I tried to profile these two approaches because I couldn't think of any other way to answer this question wrt performance. The difference is negligible. I found that the order of magnitude of the execution time was the same for both approaches.

            I first created 97 threads (itemsInInventory = 100) and let each of them call removeItem(). Then I tried the same experiment with 997 threads (itemsInInventory = 1000).

            I compared :


            Result: Both JProfiler and Eclipse TPTP gave different execution times for both samples. But within each tool, both code samples compared roughly equal.

            NOTE: Using tryLock() instead of lock() for this experiment is not a good idea because the former doesn't block the thread until it gets the lock.

            1. OK, let's just leave it the way it is then.

  5. I added some additional explanation of what constitutes a composite operation. This is a little tricky because:

    int a = b;

    is a composite operation:

    whereas to the best of knowledge

    int a = 4 + 7 / 2

    is not, because the the value of a does not depend upon an existing value and presumably the rhs of this expression reduces to a constant value which is simply written to a as an atomic operation.

    1. Is there any reasoning behind a = b being a composite operation? Volatile is typically used to solve the visibility issue here. As volatile does not work for composite operations, this contradicts that a = b is a composite operation. OTOH, a = 4 + 7 / 2 is also not a composite operation as you say.

      1. a = b is a composite operation because it involves more than one discrete operation:

        1. the value of b is read
        2. the value of a is written

        A separate thread and modify the value of b, so that at the end of this operation a != b.

        Basically anything that cannot be implemented as a single machine instructions (on all supported architectures) is a composite operation.

        1. Perhaps composite should be changed to compound in this guideline.

          compound operations : +=, /=, ... (according to JLS these are compound assignment operators). var ++ could also fall into this category as it is var = var + 1 which is equivalent to var += 1.

          a = b may fit your definition of composite operation, however, IMO this guideline should only focus on atomicity of composite operations where the result of the operation depends on the value of the left hand operand. This is because the a = b case is covered by CON00. This guideline strongly recommends against the use of volatile for compound operations.

    • The 'block synchronization' works, and the blurb is correct. But the code is not actually any faster than the method-synchronized CS, since the method consists of only a synchronized block. The only advantage you gain is the private lock.
    • Given CON04-J, I wonder if every CS that uses method synchronization needs to be accompanied by a CS that is identical except for using block synchronization. In those cases, we should just drop the method sync. CS.
    • Similarly, can we bundle the 'use a reentrant lock' CS's into its own rule? Unless there is a good reason why re-entrency is worthwhile here, I would drop the reentrant CS. (The code is currently bad anyway...if the lock fails, the function does nothing with no indication.)
    • This rule has a weakness which needs to be acknowledged. To securely do any math, you need to range-check your values. But range-checking before operating creates TOCTOU race conditions. And the TOCTOU race conditions distract from the rule, which is supposed to be about the fact that composite operators like ++ and += look atomic, but aren't.

    Perhaps we should make examples around bitwise operators like |= and || rather than arithmetic operators. That way we don't have to worry about range-checking. I don't mind having one NCCE involving arithmetic just to be comprehensive. But two seems to make the rule awfully long.

      • True. But in general, when using block synchronization it is likely that code that does not require synchronization can be safely moved out of the synchronized block. This improves throughput. I think the text ought to make it clear that this CS has no obvious performance benefits over the method synchronization one because the amount of enclosed code is the same, but in general it may be more useful.
      • CON04 does not ban method synchronization. It is largely applicable to public classes. Package-private classes are permitted. In this example, the class is package-private. So no need to drop the CS.
      • Yes, I note that nothing is done if the lock fails, however this might be "fixed" by using lock() instead of tryLock(). I just wanted to show an alternative implementation. This can be pointed out in the CS. Using tryLock() puts the burden on the client - it must ensure that the item is removed by calling the method until it succeeds in acquiring the lock. Perhaps it needs a Boolean return type to indicate the result of tryLock() being (un)successful. If we remove this CS from here, I doubt there is another place to put it and by far, it is a valid CS.
      • I am going to add AtomicInteger to the list of overflow susceptible types. Then we can reference the integer overflow guideline from here. I think the current examples serve as a good reminder of how to use atomic utilities to prevent overflow. (The reader can skip trying to understand the overflow check and read the main point)

      Note that this guideline does not talk about primitive variables or ++ and -- alone, anymore. The title is more generalized to include "shared variables". It is going to be much harder to explain in the text if you use bitwise operators (you would need to explain results, unexpected values and so on...the reader has to then understand the expected/unexpected output possibly after doing some binary arithmetic).

      I added - Noncompliant Code Example (addition, atomic integer fields), based on Tim's suggestion that throwing atomic variables in doesn't solve the problem either. The guideline is long, however, the examples are useful.

      1. True. But in general, when using block synchronization it is likely that code that does not require synchronization can be safely moved out of the synchronized block. This improves throughput. I think the text ought to make it clear that this CS has no obvious performance benefits over the method synchronization one because the amount of enclosed code is the same, but in general it may be more useful.

        I've modified the text in that CS to explain some of the nuances.

        1. I think for the time being we should eliminate tryLock() and replace it with lock(). The tryLock() discussion can be moved out to LCK07-J. Avoid deadlock by requesting and releasing locks in the same order as you suggested.

          1. I have added a NCCE/CS set that focuses on the non-atomicity of bitwise AND & OR. I used these operations because they cannot cause overflow, so a NCCE can comply with integer rules and other concurrency rules, violating only this rule. I will suggest that we get rid of the increment/decrement NCCE/CS's, as they are more complex, and provide no additional information (except items covered by other rules such as CON04-J).

            Also, I have added a ReentrantLock CS to CON12-J. I think ReentrantLock is more useful in that rule than in this one, and would suggest taking it out of this rule.

            1. David, your examples are valid but here are my first thoughts:

              I still prefer the integer examples because:

              • They model common day-to-day requirements better and provide a more interesting read than something you'll rarely see. The new examples are arguably more complex than the itemsInInventory examples simply because with bitwise one has to understand what's going on - it's not as intuitive as decrementing a variable.
              • Bit manipulation is not as popular as other languages in Java to the best of my knowledge, in particular your example. Again, using bit level flags (to the best of my knowledge) has no significant performance benefits. Memory management wise, you are creating a static byte for each flag anyway. My vote goes out for maintainability and avoiding the mess.
              • You should use the enum type (and EnumSet etc. depending on what you're doing) for the examples instead of the public static final fields. (Effective Java, item 32,30). Should you use these, the reader must know about the enum type and no other rule discusses it (yet).
              • It is fine to state that an NCE violates another rule such as integer overflow so the itemsInInventory examples are allowed to contain a minor side-effect.
              • It reminds the reader by reinforcing that overflow is possible with concurrent utilities
              • So that this rule is not operator specific (+=) but also about get() and set() problems with atomic* fields
              • The code samples are longer than the itemsInInventory examples

              I am getting a bit doubtful about compliant solutions in general. What would happen if someone uses the Reentrant locks for solving this problem? The code is still compliant with this guideline. What happens if we don't list it as a compliant solution?

              1. Also, the Compliant Solution (java.util.concurrent.atomic classes) for the bitwise CS doesn't check whether the flag argument is negative. Integer promotion may occur and cause erroneous results (upper 16 bits may not get cleared). See INT06-J. Do not use bitwise operators on integers incorrectly. And unfortunately there are no atomic classes for bytes. If it installs this check then it is equivalent to the current examples. Your other options are to accept an int instead of a byte or follow INT06.

                EDIT: Here's an example BooleanInversion that might be worth considering.

                1. David, I've hidden the flags bitwise example that you added in case you'd like to use it again. I have added my example of a compound operation that is thread-unsafe and which does not contain a TOCTOU. I still feel that itemsInInventory should go somewhere to demonstrate the integer overflow problem. I think I am open to moving it to the INT section. The example that I have just added is shorter than both. Comments?

  6. I think the example code at the top,

    is not a good introductory example for this item. This code is highly confusing even without concurrent access. Also concurrent access is only possible with fields and the code makes it look like x is a local variable. This is really an example of where widening primitive conversion gives seeming odd results. The type conversions are:

    Again, this is kinda muddled even without concurrency.

    1. I think I second Tim's opinion. I am going to rename this guideline to use "compound operations" instead of "composite operations". I think this guideline should not cover composite operations because the a = b case does not quite belong here.

  7. I like the current flag NCCE/CS code; it is simpler than my bitmask code.

    In the Sync CS does flag need to be volatile?

    For the final NCCE/CS set, I'd have just:

    • an NCCE that did no overflow checking. Simplest, and we mention that it also violates an integer rule.
    • the 1st NCCE which does overflow checking. We mention that it also has a TOCTOU race condition.
    • No NCCE using volatile, we already have that in the flag NCCE/CS set.
    • the 2nd NCCE using AtomicInts
    • the 1st CS (nicknamed 'synchronize')
    • No CS about block synchronization; that is covered by another rule
    • No CS about ReentrantLock; that is covered by another rule.

    Finally, I'd remove the itemsInInventory example; it no longer provides anything not covered by the other two NCCE/CS sets.

      • See next point
      • I think it might be ok to leave a comment in the first NCE saying - "check for integer overflow", and removing the code that does this. That way we have simplified the code without technically violating a guideline.
      • Ok, we can just add some text to the preceding NCE warning about this
      • Ok
      • Ok
      • Agreed. I think all this information about block/method synchorization should go in the introductory text. Then we can mention that block synchronization can be used alternatively.
      • Will think about this one.

      Ok, I could remove the itemsInInventory example and use it in the integer overflow example. However, we will lose a valuable table then.

  8. I'm not 100% sure about the Compliant Solution (java.util.concurrent.atomic.AtomicBoolean) code example – it seems a odd use of AtomicBoolean. I posted a question to concurrency-interest about this implementation. I think the below is a cleaner implementation (that I'm sure is actually atomic).

    Internally, AtomicBoolean uses an int so I'm not sure why toggleAndGet is not a method on AtomicBoolean. Seems odd to me so I asked.

    1. The AtomicBoolean CS does have a race window after the CompareAndSet and before returning the value. So toggle() always toggles the flag, but may return the wrong value. The way to fix this is to return temp rather than calling get().

      1. David, I am aware of the race window. However, if you return temp as I did earlier, you might be working with a stale value. I think the easiest way to solve this is to provide a different getter to retrieve the value of the flag. Trying to combine these operations is risky. Other than that, I am not sure what's wrong...probably need to look at AtomicBoolean's code.

        1. Well, the CS is merely a workaround to handle the non-atomicity of the API it is working with. It doesn't promise atomiciticy itself. You can either have atomicity & locking (which might be expensive) or you run the risk of returning stale values....it's a trade-off. Which should be explicitly stated in the CS. Since we aren't doing locking, I'm inclined to go with the stale value.

          1. I've updated the code examples based on my previous comment. Please review.

            EDIT: However, we might still want to revert and show the toggleAndGet() method. Let's wait for responses to Tim's query on the concurrency-interest list before deciding.

  9. Dhruv Comment: I can keep the suggestion to synchronize both methods but I have added a line about using volatile too because it is a valid CS...and for the sake of completeness

    For reference, the below two implementations:

    This statement is a bad idea. The second implementation mixes locking models with the use of volatile fields which is confusing and error prone. I would argue it is wrong, however, what is really wrong here is that the example has an ill-defined and non-obvious class invariant.

    The top solution uses locking to ensure atomicity of the toggle and any reads. Locking is used both to ensure atomicity and visibility of changes.

    The second solution mixes this, atomicity of changes is assured via locking and visibility through the use of a volatile field. Two toggles are atomic from each other, however the getFlag() method can "see" into the atomic operation and yank out either the prior or new value of the field.

    I think the problem may be that the example is just too simple to illustrate this point.

    1. The 2nd code sample (that Tim does not like) is known as the 'cheap read-write lock trick', and is documented in a Compliant Solution at VNA06-J. Do not assume that declaring an object reference volatile guarantees visibility of its members

      EDIT: Any arguments about the viability of the cheap read-write lock trick should go in the comments section for CON11-J.

      1. I've never heard that name, thanks for the pointer.

      2. Here's my take on this:

        • The cheap read/write lock trick is not a violation of this guideline which is about atomicity of compound operations. Neither is it an exception to this guideline or a compliant solution.
        • This technique helps reduce the danger of deadlocks by limiting synchronization for a method.

        I see what Tim means about updates being slightly out of order with reads. However, the recommendation of this relatively recent article Goetz 07, Pattern #5 makes me unsure whether this is "compliant" or "wrong".

        1. Brian did not really recommend this approach, he just brings it up as an advance approach that is applicable in some very narrow circumstances. I would really argue with him that ReadWriteLock is probably a better a way better, and more clear, solution in the circumstances he presents this solution. I'll have to check the performance impact...that could be an issue.

          To his credit, Brian included several paragraphs of warnings and caveats. I'd recommend if you include this brittle solution it (1) be later and marked as advanced (2) discuss the very limited circumstances where it is appropriate.

          1. I think the only place I find this admissible is - in a getter that has only a return statement. Specifically, when reads occur more frequently than writes. And that's why I am unable to call this a flawed approach. I've added a line to the same CS that says - this advanced trick is brittle/unsafe for all other uses. I hope this addresses your suggestions. I think another CS is unwarranted because this issue has nothing to do with ensuring atomicity of ^=.

  10. empty comment, please ignore.

  11. Compound operations on shared variables must be performed atomically to prevent data races and race conditions.

    I'm confused to the difference between race conditions and data races. AFAICT 'race condition' describes the general property, and 'data race' is a race condition that occurs in the JVM. In particular the only distinction between the two is visibility; a 'data race' can occur because of stale values. Race conditions have nothing to do with visibility while data races can be intertwined with visibility. But the definitions (in our BB. Definitions section does not make this clear.

    I'm recommending that we either modify the quoted section in this rule's introduction to remove data races, or we make the distinction between data races & race conditions more clear in the definitions. I would prefer the latter option.

    1. On another note, I wonder if we should provide a definition for TOCTOU. Or replace the term in this rule with check-then-act?

      1. Agreed. The definitions section has several definitions of data race/race conditions but a simple sentence might help. We might want to include TOCTOU in the definitions as many other guidelines also use the term.

  12. I'm wondering if we have done the right thing with how we qualify these names:

    Compliant Solution (java.util.concurrent.atomic.AtomicBoolean)

    We end up repeating this very long qualified name a couple of times, but then the code won't compile because we use the unqualified name in the code.

    I'm looking at the Sun Java tutorial page a for atomic variables and it does the opposite. I'm going to change; let me know what you think because we'll probably want to apply this consistently.

    1. I would agree, simply because many classes are clearly in this category (Atomic*). Qualifying other classes (eg Lock) can be done once in various paragraphs, but doesn't need to be in headings.

    2. Ok, we can remove the qualification from the headings. However, I think it could be used at least once in the text while introducing the class for the first time. I suspect you didn't mean to add all the required import statements to the examples. If a class with the same name does not exists in multiple packages, then doing a simple "Organize imports" automatically imports the packages. Usually any ambiguity in selecting a class from multiple packages can be resolved by knowing the context of the example. So far we've been assuming import statements may be forgone to save space. The code would compile with this automatic import facility.

      1. ok, that sounds like a reasonable approach

  13. There are some significant issues with the Adder example at the end that need to be fixed. My biggest problem is the lack of continuity through the three examples, that is, the first Adder just adds two numbers but the second adds an integer overflow check (which, BTW, I believe is wrong) and the CS also adds the overflow check.

    The first requirement is that we can't have the example changing in the middle like this, so all three examples must be the same.

    I think the better approach would be to eliminate overflow check and just stay with the initial, simpler example. I modified the first adder NCE to explain the lack of overflow checking in a way that makes the race condition a little more compelling (I think). I suggest we carry this example through to the other two adder examples.

      • Why is the overflow check incorrect? (going to check the comment in other guideline)
      • Where would the check-then-act (toctou) problem that uses the integer overflow example go then? [Goetz 06] treats all the NCE cases uniformly by calling them compound actions. We want to talk about data races as well as race conditions; the solutions are the same.
      1. I think it is important to show that while adding can be done atomically (as long as the memory location containing the sum is independent of the two adding numbers), adding with overflow checks require a good deal more work to avoid TOCTOU. Currently the 2nd NCCE adds both the AtomicInteger class and overflow checking.

        Suggest we decompose this problem into:

        • A NCCE that adds two ints w/no overflow checking, and uses a setA and setB method. You could argue that such an example provides thread-safety (but not integer safety).
        • A CCE with some locking (synchronized methods, perhaps, or volatile fields). Provides thread-safety but no integer safety.
        • A NCCE that adds overflow checks to the 1st NCCE. Non-thread-safe, but integer-safe
        • A NCCE with overflow checks & AtomicInts.
        • The CCE with synchorinized methods & overflow checks.
      2. I cannot tell the differenc ebetween a data race and a race condition. Sometimes I think that a data race only involves one shared variable and consequently is a specific instance of a race condition, but the definitions we have do not support this.

        Similarly, a TOCTOU or check-then-act is not particuarly different from any other compound operation where the successfull completion of an operation depends on multiple variables, that is, it is a specific instance of a race conditoin. It sounds like I agree with Goetz.

        I think it is worth while to have examples of TOCTOU somewhere in this standard, but I think it is a little more interesting when we are collecting some information about a stateful but opaque object, such as a file, that can change state between the the check and the access.

        I'm not sure how many examples we need to demonstrate this perhaps we can talk about this Monday. Right now I'm thinking we should keep it simple.

        1. Let's resolve this on Monday. I suggest going through the December 2008 and May 2008 archives of the JMM mailing list for now.

  14. The second table under Automatic Detection is empty. Should it be filled in or deleted?

  15. Few editorial suggestions/questions (only for consideration, no reply needed):

    • The last CS&#39;s text does not match the NCE any longer in that, it has setValues(1, 1) instead of setValues(Integer.MAX_VALUE, 0)

    rcs> we still need to figure out what we are doing with this last example

    • I suspect the &quot;a&quot; in bold could be forgone.

      Read-write locks allow a shared state to be accessed by multiple readers or a single writer but never both.

    rcs> gone

    • &quot;The guideline CON11-J. Do not assume that declaring an object reference volatile guarantees visibility of its members also addresses the volatile-read, synchronized-write pattern.&quot; . Insert words in bold.

    rcs> i think it is fine as is.

    • &quot;getSum() might return 0 or Integer.MAX_VALUE&quot; - Integer.Max should be in code font
    • Do we use the quote tags for one-line quotes too? i.e. the Goetz 06 quote.

    rcs> it is stylistic if we use block quotes (the quote tags) or quotation marks.

    • The following sentence should probably occur after the sentence &quot;For instance, a thread can call setValues() ...&quot; because the &quot;for instance&quot; supports the first point and not this one:

    Furthermore, in the absence of synchronization, data races occur in the check for integer overflow.

    Also, we could get rid of the &quot;Even worse,&quot; in the next starting para.

    • &quot;This compliant solution synchronizes the setValues() and getSum() methods to ensure atomicity.&quot;

    Is the following better:

    &quot;This compliant solution synchronizes the methods setValues() and getSum() to ensure atomicity.&quot;

    The former could mean that there are several setValues() and getSum() methods.

    • The risk assessment could be reworded as:

    For example, information about a previous user session can be inadvertently disclosed to a distinct user because of the visibility of stale values.

    • &quot;... depending on which thread obtains the intrinsic lock first.&quot;


    &quot;depending on the thread that obtains the intrinsic lock first.&quot;

  16. Hm, the AtomicInteger NCCE of CON01-J is more suited to CON07-J rule than that one. (I'm not saying we should take it out of CON01-J, but perhaps we should add a ref to CON07-J at the NCCE)

    1. We have a ref to Con07 in the intro and I was hoping that is sufficient. If we use the operator + then it belongs here, if we use the method getAndAdd() then it probably belongs in CON07. Furthermore, it seems a little unusual to use atomic ints + synchronization together in the CS. At this point I am tempted to use getAndAdd() and move this to CON07.

      1. I think the last CS should just use normal variables and synchronization and not atomic ints, as before. I'll make this change.

        EDIT: I fixed the CS. Anyway, now I don't agree with the text of the NCE with atomic ints:

        This does not eliminate the data race because compound operation a + b is still non-atomic.

        because I would argue that the class does not use atomic int facilities properly i.e. getAndAdd() in getSum(). So this problem can very well be solved using atomic ints.

        What we should point out is that there is no data race, however, there is a race condition in that setValues() is not synchronized and consequently, even though getSum() will be atomic, it will return an incorrect sum.

        1. I've changed "data race"=>"race condition".

          I don't see any problem saying that the atomic int example is noncompliant. We are not saying that atomic integers cannot be used in the solution, we are only saying that this particular code example is noncompliant because it doesn't solve the problem.

          Anyway, I tried to rephrase it to make it very clear that the "The simple replacement of the two int fields with atomic integers in this example does not eliminate the race condition"

          1. Ok, but it might help to add: "The getSum() method can be fixed by using AtomicInteger.getAndAdd(), however, the example is still noncompliant because setValues() is non-atomic.". Note that setValues() is also a compound operation, "update values".

            1. honestly I think it is fine the way it is. As soon as you start writing code, your 90% of the way towards adding another noncompliant code example.

              1. Promoting this to draft; since no one wants other changes perhaps this should be draft-one.

  17. I wonder why "synchronized" is needed in the toggle() method of the read-write lock example.

    1. The volatile ensures memory visibility, the synchronized ensures that the mutation is atomic.

      is a read of the flag field, followed by an xor, followed by a write of the flag field. Without a synchronized on the method concurrent invocations of the method could cause some to be "missed" in the sense that they appear to have no effect.

      1. I think danielluo was talking about the juc read-write lock which should not require "synchronized". I've removed it.

  18. Possibly add a further compliant example that moves all the state into an immutable object with a volatile/AtomicReference to that object:

    1. Thomas's example should be a compliant solution. I'd recommend that it annotate the intent that Operands is immutable: