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
:
final class Flag { private boolean flag = true; public void toggle() { // Unsafe flag = !flag; } public boolean getFlag() { // Unsafe return 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:
Time | flag= | Thread | Action |
---|---|---|---|
1 | true | t1 | Reads the current value of |
2 | true | t2 | Reads the current value of |
3 | true | t1 | Toggles the temporary variable to false |
4 | true | t2 | Toggles the temporary variable to false |
5 | false | t1 | Writes the temporary variable's value to |
6 | false | t2 | Writes the temporary variable's value to |
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
:
final class Flag { private boolean flag = true; public void toggle() { // Unsafe flag ^= true; // Same as flag = !flag; } public boolean getFlag() { // Unsafe return 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:
final class Flag { private volatile boolean flag = true; public void toggle() { // Unsafe flag ^= true; } public boolean getFlag() { // Safe return flag; } }
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:
final class Flag { private boolean flag = true; public synchronized void toggle() { flag ^= true; // Same as flag = !flag; } public synchronized boolean getFlag() { return flag; } }
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:
Time | flag= | Thread | Action |
---|---|---|---|
1 | true | t1 | Reads the current value of |
2 | true | t1 | Toggles the temporary variable to false |
3 | false | t1 | Writes the temporary variable's value to |
4 | false | t2 | Reads the current value of |
5 | false | t2 | Toggles the temporary variable to true |
6 | true | t2 | Writes the temporary variable's value to |
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.
final class Flag { private volatile boolean flag = true; public synchronized void toggle() { flag ^= true; // Same as flag = !flag; } public boolean getFlag() { return flag; } }
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:
final class Flag { private boolean flag = true; private final ReadWriteLock lock = new ReentrantReadWriteLock(); private final Lock readLock = lock.readLock(); private final Lock writeLock = lock.writeLock(); public void toggle() { writeLock.lock(); try { flag ^= true; // Same as flag = !flag; } finally { writeLock.unlock(); } } public boolean getFlag() { readLock.lock(); try { return flag; } finally { readLock.unlock(); } } }
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
:
import java.util.concurrent.atomic.AtomicBoolean; final class Flag { private AtomicBoolean flag = new AtomicBoolean(true); public void toggle() { boolean temp; do { temp = flag.get(); } while (!flag.compareAndSet(temp, !temp)); } public AtomicBoolean getFlag() { return flag; } }
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).
final class Adder { private int a; private int b; public int getSum() { return a + b; } public void setValues(int a, int b) { this.a = a; this.b = b; } }
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:
final class Adder { private final AtomicInteger a = new AtomicInteger(); private final AtomicInteger b = new AtomicInteger(); public int getSum() { return a.get() + b.get(); } public void setValues(int a, int b) { this.a.set(a); this.b.set(b); } }
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:
final class Adder { private int a; private int b; public synchronized int getSum() { // Check for overflow return a + b; } public synchronized void setValues(int a, int b) { this.a = a; this.b = b; } }
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.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
VNA02-J | Medium | Probable | Medium | P8 | L2 |
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.
Tool | Version | Checker | Description |
---|---|---|---|
CodeSonar | 4.2 | FB.MT_CORRECTNESS.IS2_INCONSISTENT_SYNC FB.MT_CORRECTNESS.IS_FIELD_NOT_GUARDED FB.MT_CORRECTNESS.STCAL_INVOKE_ON_STATIC_CALENDAR_INSTANCE FB.MT_CORRECTNESS.STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE FB.MT_CORRECTNESS.STCAL_STATIC_CALENDAR_INSTANCE FB.MT_CORRECTNESS.STCAL_STATIC_SIMPLE_DATE_FORMAT_INSTANCE | Inconsistent synchronization Field not guarded against concurrent access Call to static Calendar Call to static DateFormat Static Calendar field Static DateFormat |
Coverity | 7.5 | GUARDED_BY_VIOLATION | Implemented |
Parasoft Jtest | 2024.1 | CERT.VNA02.SSUG CERT.VNA02.MRAV | Make the get method for a field synchronized if the set method is synchronized Access related Atomic variables in a synchronized block |
PVS-Studio | 7.34 | V6074 | |
ThreadSafe | 1.3 | CCE_SL_INCONSISTENT | Implemented |
Related Guidelines
CWE-366, Race Condition within a Thread |
Bibliography
[API 2014] | |
Item 66, "Synchronize Access to Shared Mutable Iata" | |
Section 2.3, "Locking" | |
[JLS 2015] | Chapter 17, "Threads and Locks" |
[Lea 2000] | Section 2.1.1.1, "Objects and Locks" |
80 Comments
Dhruv Mohindra
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 usingvolatile
.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.
David Svoboda
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 ifitemsInInventory
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.Dhruv Mohindra
private
. The NCE is vulnerable to all kinds of race conditions as noted in the guideline.Robert Seacord
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.
Dhruv Mohindra
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.
Robert Seacord (Manager)
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.
Dhruv Mohindra
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?
Tim Halloran
Yes, however we need some work to get this to compile
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 againstMAX_INVENTORY
If
MAX_INVENTORY
is defined as I did asInteger.MAX_INTEGER
this code can be compiled down tobecause 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 totrue
(assuming, of course, that the VM has assertions enabled).Thoughts?
Dhruv Mohindra
Tim,
Here are my thoughts:
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.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.Tim Halloran
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()
andgetLast()
orjava.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)
Dhruv Mohindra
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. UsingunderStock()
abstracts the nonrelevant details from this guideline.Tim Halloran
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 ofx
, adding one to it, and writing the new, incremented value back tox
.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:Time
itemsInInventory=
Thread
Action
1
100
t1
reads the current value of
itemsInInventory
, 100, into a temporary variable on the thread's stack2
100
t2
reads the current value of
itemsInInventory
, (still) 100, into a temporary variable on the thread's stack3
100
t1
decrements the temporary variable to 99
4
100
t2
decrements the temporary variable to 99
5
99
t1
writes the temporary variable value to
itemsInInventory
6
99
t2
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 toreturnItem()
or concurrent calls toremoveItem()
andreturnItem()
.Dhruv Mohindra
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()
andoverStocked()
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?Tim Halloran
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.
Dhruv Mohindra
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()
andoverStocked()
asprivate
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!
Robert Seacord
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.
Tim Halloran
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.
Dhruv Mohindra
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
methodunderStocked()
from the else condition. This method could request more inventory and choose to throw anIllegalStateException
if it failed. Not a bad approach - though it would not compile without providing the implementation ofunderStocked()
.Robert Seacord (Manager)
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.
Dhruv Mohindra
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.
Robert Seacord (Manager)
No, here is what I meant:
This should synchronize less code, because the creation of the
IllegalStateException
object takes place outside the synchronized block.Dhruv Mohindra
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 eliminatedreturnItem()
, 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 longif
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.
Robert Seacord (Manager)
> 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.
Robert Seacord (Manager)
No, here is what I meant:
This should synchronize less code, because the creation of the
IllegalStateException
object takes place outside the synchronized block.Dhruv Mohindra
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 :
and,
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.
Robert Seacord
OK, let's just leave it the way it is then.
Robert Seacord (Manager)
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.Dhruv Mohindra
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.
Robert Seacord (Manager)
a = b
is a composite operation because it involves more than one discrete operation: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.
Dhruv Mohindra
Perhaps
composite
should be changed tocompound
in this guideline.compound operations : +=, /=, ... (according to JLS these are compound assignment operators).
var ++
could also fall into this category as it isvar = var + 1
which is equivalent tovar += 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.David Svoboda
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.
Dhruv Mohindra
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.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.
David Svoboda
I've modified the text in that CS to explain some of the nuances.
Dhruv Mohindra
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.
David Svoboda
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.
Dhruv Mohindra
David, your examples are valid but here are my first thoughts:
I still prefer the integer examples because:
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?
Dhruv Mohindra
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.
Dhruv Mohindra
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?
Tim Halloran
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.
Dhruv Mohindra
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.
David Svoboda
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:
Finally, I'd remove the itemsInInventory example; it no longer provides anything not covered by the other two NCCE/CS sets.
Dhruv Mohindra
Ok, I could remove the itemsInInventory example and use it in the integer overflow example. However, we will lose a valuable table then.
Tim Halloran
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 anint
so I'm not sure whytoggleAndGet
is not a method onAtomicBoolean
. Seems odd to me so I asked.David Svoboda
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().
Dhruv Mohindra
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.David Svoboda
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.
Dhruv Mohindra
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.
Tim Halloran
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.
David Svoboda
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.
Tim Halloran
I've never heard that name, thanks for the pointer.
Dhruv Mohindra
Here's my take on this:
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".
Tim Halloran
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.
Dhruv Mohindra
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 ^=.
Robert Seacord
empty comment, please ignore.
David Svoboda
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.
David Svoboda
On another note, I wonder if we should provide a definition for TOCTOU. Or replace the term in this rule with check-then-act?
Dhruv Mohindra
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.
Robert Seacord
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.
David Svoboda
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.
Dhruv Mohindra
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.
Robert Seacord (Manager)
ok, that sounds like a reasonable approach
Robert Seacord (Manager)
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.
Dhruv Mohindra
David Svoboda
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:
Robert Seacord
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.
Dhruv Mohindra
Let's resolve this on Monday. I suggest going through the December 2008 and May 2008 archives of the JMM mailing list for now.
Pennie Walters
The second table under Automatic Detection is empty. Should it be filled in or deleted?
Dhruv Mohindra
Few editorial suggestions/questions (only for consideration, no reply needed):
rcs> we still need to figure out what we are doing with this last example
rcs> gone
rcs> i think it is fine as is.
rcs> it is stylistic if we use block quotes (the quote tags) or quotation marks.
Also, we could get rid of the "Even worse," in the next starting para.
Is the following better:
"This compliant solution synchronizes the methods setValues() and getSum() to ensure atomicity."
The former could mean that there are several setValues() and getSum() methods.
rephrased:
"depending on the thread that obtains the intrinsic lock first."
David Svoboda
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)
Dhruv Mohindra
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.
Dhruv Mohindra
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:
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.
Robert Seacord
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"
Dhruv Mohindra
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".
Robert Seacord (Manager)
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.
Dhruv Mohindra
Promoting this to draft; since no one wants other changes perhaps this should be draft-one.
danielluo
I wonder why "synchronized" is needed in the toggle() method of the read-write lock example.
Tim Halloran
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.
Dhruv Mohindra
I think danielluo was talking about the juc read-write lock which should not require "synchronized". I've removed it.
Thomas Hawtin
Possibly add a further compliant example that moves all the state into an immutable object with a volatile/AtomicReference to that object:
Tim Halloran
Thomas's example should be a compliant solution. I'd recommend that it annotate the intent that
Operands
is immutable: