search

cushon / issues-import

0 stars 0 forks source link

Disallow incrementing/decrementing a volatile variable #168

Closed cushon closed 9 years ago

cushon commented 9 years ago

Original issue created by eaftan@google.com on 2013-08-02 at 08:36 PM

volatile variables are usually used as signal variables in concurrent programs. Though any write to a volatile variable is immediately visible to any later read, this does not mean that non-atomic operations like increment/decrement are atomic on volatiles.

Not sure if this is always a bug or not. May be more appropriate for a warning.

cushon commented 9 years ago

Original comment posted by philipguin on 2014-04-05 at 06:18 AM

I strongly believe this should be a standard warning in Java, if not completely changed functionality. I can't imagine an uninformed programmer ever expecting increment/decrement to be non-atomic for a volatile variable, let alone ever desiring that behavior.

If only this behavior could be made atomic... sigh. Screw backwards compatibility!

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2014-04-08 at 12:59 AM

(No comment entered for this change.)

Status: Accepted Owner: eaftan@google.com

cushon commented 9 years ago

Original comment posted by phwendler on 2014-04-13 at 04:00 PM

I have used increment for volatile variables already when I was sure there was only one writing thread. Imagine a worker thread that increments a variable to keep other threads (GUI) informed about progress.

However, I guess the performance benefit compared to an AtomicInteger with a single writer is so low (if it exists at all) that it is probably not worth the error risk.

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2014-04-14 at 03:00 AM

I implemented this check and ran it over Google's code. The vast majority were errors, but there were a few cases where either increment/decrement only occurs when a lock is held, or the author was sure there was only one writing thread and documented this in a comment. For the first case, I've excepted any code that is inside a synchronized block. For the second case, I would suggest they use @SuppressWarnings to turn off the check and add a comment explaining why the code is correct.

cushon commented 9 years ago

Original comment posted by eaftan@google.com on 2014-06-05 at 06:08 PM

Check committed as a warning in revision 717daf6b0ae0.

Status: Fixed