Synchronizing on AtomicBoolean?

Question

In an application I'm working on I found the following code snippet:

public class MyClass {

    private AtomicBoolean atomicBoolean = new AtomicBoolean(false);

    public void Execute() {
        // Whole lot of business logic
        // ....
        synchronized (this.atomicBoolean) {
            // Want to make sure that execution is stopped if Stop() was called
            if (this.atomicBoolean.get()) {
                throw new SpecificException("...");
            }
            // Some more business logic...
         }
     }

    public void Stop() {
        synchronized (this.atomicBoolean) {
            this.atomicBoolean.set(true);
        }
    }
}

According to FindBugs this is not correct as I can't use an AtomicBoolean together with synchronized and expect it to block the object.

My question is: What is the correct way to rewrite this methods? I've read about using an lock Object together with a boolean attribute instead but it appears kinda clumsy to introduce two new attributes for this lock.

Edit: As stated in a comment below: I think the intention is that in the two synchronized blocks, the AtomicBoolean can't be changed and that while one Thread is in one of the synchronized blocks, none other such block could be entered.

Yes, synchronization doesn't stop the `AtomicBoolean` from being changed elsewhere, but to give a good suggestion on how to fix it we'd need to know what else these methods are trying to do. — Joachim Sauer, Apr 04 '19 at 11:11
Why don't you just make the two methods synchronized? That would be equivalent to what you seem to want to do. — ernest_k, Apr 04 '19 at 11:11
Why do you even want to use `synchronized` here. On the face of it it is entirely unnecessary as an `AtomicBoolean` already is atomic and has the necessary visibility guarantees without locking (and locking possibly leads to worse performance), so explain your reasons and what you are trying to achieve so we can explain where your thinking is wrong (or correct). In the code as shown you could better use a plain `boolean` variable instead of an `AtomicBoolean`. — Mark Rotteveel, Apr 04 '19 at 11:15
Several ways, in general if you are using stuff from the java concurrent package (which is a good thing) like AtomicBoolean, you probably don't want to be doing anything with low level primitives like synchronized or volatile directly. It has a wide range of datastructures and locks with different semantics. In this case, what you are looking for is a bit unclear. But synchronizing on the get and set of AtomicBoolean is definitely completely redundant. Code like this means that whoever wrote this doesn't understand this topic very well. — Jilles van Gurp, Apr 04 '19 at 11:18
While using an atomic variable together with `synchronized` looks suspicious, I’m wondering whether FindBugs is specifically addressing this combination or just complaining that `atomicBoolean` has not been declared `final`. The latter would be easy to fix. — Holger, Apr 04 '19 at 12:22
@Holger It's the former: _This method performs synchronization an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized. For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean. Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date._ — Jdv, Apr 04 '19 at 12:26
Well yes, and it’s right about the confusion. Here, as far as shown by your example, you could use `synchronized` without an `AtomicBoolean` or an `AtomicBoolean` without `synchronized`, and it still would work. In fact, a `private volatile boolean stop;` would do as well. — Holger, Apr 04 '19 at 12:28

score 8 · Accepted Answer · answered Apr 04 '19 at 11:11

8

just replace the synchronized (this.atomicBoolean) { part from both methods, AtomicBoolean::get and AtomicBoolean::set is already atomic.

answered Apr 04 '19 at 11:11

Eugene

117,005
15
201
306

I expect the AtomicBoolean to be manipulated further inside the synchronized blocks with the (wrong) assumption that they'd stay unmodified by other threads. If that's the case, then simply removing the synchronized blocks won't make the code correct. – Joachim Sauer Apr 04 '19 at 11:12
Thanks for the quick reply! I've edited my question a bit to provide more information. I have a question regarding your proposed solution: I think that it was expected that the value of the `AtomicBoolean` can't be modified inside the `synchronized` block in `Execute()` (`Method1()` before my edit). If I understand your answer correctly this would not be the case with your solution, would it? – Jdv Apr 04 '19 at 11:21
@Jdv it seems that this acts as an `exit` flag, once set, you have to throw an exception and be done. is that correct? – Eugene Apr 04 '19 at 11:32
@Eugene I think it's supposed to be more of an unscheduled `cancel`. If `Stop()` is called during `Execute()` the latter should not finish but throw an `Excepton`. If that's not the case, during the `synchronized` block in `Execute()` it shouldn't be possible to execute `Stop()`. – Jdv Apr 04 '19 at 11:36
@Jdv might be, but I can't read minds of the coder that made this... It seems to me that this is like preparing some work first in `Execute` then checking a flag, then if it's OK to go on - commit that work; but as said I can't read minds - if you know exactly what it is supposed to do, I could probably help.... – Eugene Apr 04 '19 at 11:46
@Eugene of course, I understand. The point of my question was more to make sure that I understand your answer correctly. It doesn't create a synchronized section, right? In this case, could the `AtomicBoolean` be used to create such a section or would I need another lock object? – Jdv Apr 04 '19 at 12:20
1

@Jdv that is a loaded question, as it implies the assumption that a synchronized section was needed. – Holger Apr 04 '19 at 12:30
@Holger yes I agree, Eugenes answer solves the initial question in this post which is why I marked it as accepted. Thanks for the help. – Jdv Apr 04 '19 at 12:41

score 1 · Answer 2 · answered Apr 04 '19 at 13:45

...I can't use an AtomicBoolean together with synchronized...

For whatever it's worth, the language allows you to synchronize on any object.

As a matter of style, some programmers prefer to only synchronize on a private object that is used for no other purpose.

private static Object foobarLock = new Object();
...
public void fooItUp(...) {
    ...
    synchronized(foobarLock) {
        ...
    }
    ...
}

...and expect it to block the object

Just to be clear, when some thread T enters a synchronized (o) {...} block, that does not prevent other threads from accessing or modifying the object o. The only thing it prevents is, it prevents some other thread U from entering a synchronized block on the same object o at the same time.

Synchronizing on AtomicBoolean?

2 Answers2