Can I eliminate nested synchronized on a private field within synchronized method

Question

I've got this legacy pseudocode:

public class Wrapper {
  private volatile Sink sink;

  public synchronized void flushSink() {
    if (sink != null) {
      synchronized (sink) {
        sink.flush();
      }
    }
  }

  public void close()  throws IOException {
    var sink = this.sink;
    if (sink != null) {
      sink.receivedLast();
    }
  }
}

My question is about nested synchronized block.

As far as I understand the mentioned block is redundant in this case, as if two threads concurrently call flushSink(), then only one of them has access to private field sink at a time. As a result the code could be simplified to

public class Wrapper {
  private volatile Sink sink;

  public synchronized void flushSink() {
    if (sink != null) {
      sink.flush();
    }
  }
}

with possible further elimination of racy read into

public class Wrapper {
  private volatile Sink sink;

  public synchronized void flushSink() {
    var sink = this.sink;
    if (sink != null) {
      sink.flush();
    }
  }
}

So my question is whether this kind of refactoring correct in terms of synchronization and JMM and if it is (alternatively if it is not) - is there any JCStress-based test proving it?

So unless there's some other way to access/modify `sink`, outside of synchronization on the containing `Wrapper` instance (e.g. other methods in `Wrapper` that aren't synchronized), the inner synchronization block doesn't make sense. As to the "elimination of racy reads", your synchronized methods are already doing that. — Taylor, Nov 25 '22 at 15:46
I think you did not provide the complete code snippet - how is the `sink` attribute set? — Adam Siemion, Nov 25 '22 at 15:51
@AdamSiemion the code of `flushSink()` method is full, no other logic in that method — Sergey Tsypanov, Nov 25 '22 at 16:02
The code is not full, how does `sink` get initialized? @SergeyTsypanov — markspace, Nov 25 '22 at 16:05
It's initialized in constructor and set to null in close method, updated the snippet. Pay attention, that `close()` is not synchronized. — Sergey Tsypanov, Nov 25 '22 at 16:13
We'd need to see the constructor, otherwise the code you're changing might be wrong. — markspace, Nov 25 '22 at 16:23

score 1 · Answer 1 · answered Nov 25 '22 at 16:49

As far as I understand the mentioned block is redundant in this case, as if two threads concurrently call flushSink(), then only one of them has access to private field sink at a time. As a result the code could be simplified to

It is redundant as long as there is no other place in the code where sink is accessed within a synchronized block.

For instance, if the close() method were changed to the following,

public void close()  throws IOException {
  var sink = this.sink;
  if (sink != null) {
    synchronized(sink) {
      sink.receivedLast();
    }
  }
}

then the synchronized block in flushSink() would not be useless as it would ensure that receivedLast() and flush() are not being called at the same time.

score 0 · Accepted Answer · answered Dec 09 '22 at 08:23

Finally I found open source code exactly repeating the case, see java.io.PipedOutputStream:

@Override
public synchronized void flush() throws IOException {
    if (sink != null) {
        synchronized (sink) {
            sink.notifyAll();
        }
    }
}

The code looks exactly like the snippet I was investigating, and if I do the same refactoring PipedOutputStream-related JDK tests will fail.

The transformation is illegal, because synchronized on flush() method assumes locking on this i.e. on PipedOutputStream object, and synchronized (sink) locks on another object. Thus we have two different locks and lock coarsening is not allowed in this case.

Can I eliminate nested synchronized on a private field within synchronized method

2 Answers2