Convert synchronized methods to non-blocking algorithm

Question

Just find some information about non-blocking algorithms, so want to use them in practice. I changed some code from synchronized to non-blocking, so I want to ask does I made everything right and saved previous functionality.

synchronized code:

protected PersistentState persistentState;
protected ClassConstructor(final ID id)
{
    super(id);
    this.persistentState = PersistentState.UNKNOWN;
}
public final synchronized PersistentState getPersistentState()
{
    return this.persistentState;
}

protected synchronized void setPersistentState(final PersistentState newPersistentState)
{
    if (this.persistentState != newPersistentState)
    {
        this.persistentState = newPersistentState;
        notifyPersistentStateChanged();
    }
}

my alternative in non-blocking algorithm:

     protected AtomicReference<PersistentState> persistentState;
  protected ClassConstructor(final ID id)
    {
        super(id);
        this.persistentState = new AtomicReference<PersistentState>(PersistentState.UNKNOWN);
    }
   public final PersistentState getPersistentState()
    {
        return this.persistentState.get();
    }

    protected void setPersistentState(final PersistentState newPersistentState)
    {
        PersistentState tmpPersistentState;
        do
        {
            tmpPersistentState = this.persistentState.get();
        }
        while (!this.persistentState.compareAndSet(tmpPersistentState, newPersistentState));
        // this.persistentState.set(newPersistentState); removed as not necessary 
        notifyPersistentStateChanged();
    }

Do I've done everything correctly, or I missed something? Any suggestions for the code and using non-blocking method for setting abject in general?

you should not use "this.persistentState.set(newPersistentState)" as compareAndSet updates value of persistentState — hahn, Apr 18 '16 at 15:17
The compareAndSet will break out of the while loop only after setting the value to newPersistentState and hence setting it again to newPersistentState is not required as hahn mentioned. If it were required then this would be a check-then-act way which is prone to stale data issues. Thankfully it is not. — MS Srikkanth, Apr 18 '16 at 15:25

OldCurmudgeon · Accepted Answer · 2016-04-18T15:40:46.873

Depends what you mean by thread-safe. What do you want to happen if two threads try to write at the same time? Should one of them chosen at random be chosen as the correct new value?

This would be it at it's simplest.

protected AtomicReference<PersistentState> persistentState = new AtomicReference<PersistentState>(PersistentState.UNKNOWN);

public final PersistentState getPersistentState() {
    return this.persistentState.get();
}

protected void setPersistentState(final PersistentState newPersistentState) {
    persistentState.set(newPersistentState);
    notifyPersistentStateChanged();
}

private void notifyPersistentStateChanged() {
}

This would still call notifyPersistentStateChanged in all cases, even if the state hasn't changed. You need to decide what should happen in that scenario (one thread makes A -> B and another goes B -> A).

If, however, you need to only call the notify if successfully transitioned the value you could try something like this:

 protected void setPersistentState(final PersistentState newPersistentState) {
    boolean changed = false;
    for (PersistentState oldState = getPersistentState();
            // Keep going if different
            changed = !oldState.equals(newPersistentState)
            // Transition old -> new successful?
            && !persistentState.compareAndSet(oldState, newPersistentState);
            // What is it now!
            oldState = getPersistentState()) {
        // Didn't transition - go around again.
    }
    if (changed) {
        // Notify the change.
        notifyPersistentStateChanged();
    }
}

I need to notify only when changed. Btw, Do I understand well that in my case if thread will want to set same value I will get infinite loop ? — Edgar, Apr 18 '16 at 17:15
@Edgar - no. Your loop would just work fine if tmoPersistentState is the same as newPersistentState. As long as this.persistentState is same as tmpPersistentState it will return true and breakout. There shouldn't be an infinite loop. — MS Srikkanth, Apr 18 '16 at 18:24

Convert synchronized methods to non-blocking algorithm

1 Answers1