Mockito 5 deadlock when spying a synchronized method

[Edarke]

I'm migrating a codebase to Mockito 5, and I've encountered an unexpected change in behavior when transitioning to the INLINE MockMaker. Below is a simple test case that demonstrates some unintuitive behavior between the differences of spying on a synchronized method vs a method containing a synchronized block.

public class SpyTest {

  public static class SynchronizedMethod {
    public synchronized void foo() {
      System.out.println("Synchronized Method");
    }
  }

  public static class SynchronizedBlock {
    public void foo() {
      synchronized (this) {
        System.out.println("Synchronized Block");
      }
    }
  }

  private final ExecutorService executor = Executors.newCachedThreadPool();

  @Test
  @Timeout(value=1)
  public void spy_synchronizedMethod() throws Exception {
    Phaser phaser = new Phaser(2);
    SynchronizedMethod example = Mockito.spy(new SynchronizedMethod());

    Mockito.doAnswer(invocation -> {
      phaser.arriveAndAwaitAdvance();
      return invocation.callRealMethod();
    }).when(example).foo();

    Future<?> task = executor.submit(example::foo);
    executor.submit(example::foo);
    task.get(1, TimeUnit.SECONDS);
    // With Mockito 4: Test Passes because synchronization only applies to callRealMethod()
    // With Mockito 5: Timeout because second thread can't enter doAnswer block
  }

  @Test
  @Timeout(value=1)
  public void spy_synchronizedBlock() throws Exception{
    Phaser phaser = new Phaser(2);
    SynchronizedBlock example = Mockito.spy(new SynchronizedBlock());

    Mockito.doAnswer(invocation -> {
      phaser.arriveAndAwaitAdvance();
      return invocation.callRealMethod();
    }).when(example).foo();

    Future<?> task = executor.submit(example::foo);
    executor.submit(example::foo);
    task.get(1, TimeUnit.SECONDS);
    // Always Passes because `phaser.arriveAndAwaitAdvance()` is not in synchronized block
  }
}

Why This Happens

This seems to be caused by the fact that in the first class, synchronized is a flag on the method itself and Mockito 5 injects bytecode into the method and therefore into an implicit synchronized block. In the second class, the synchronized block is implemented as the first bytecode instruction of the method definition, and Mockito 5 replaces that bytecode (and the synchronized block) with the provided implementation. With the SUBCLASS MockMaker in Mockito 4, the target method is overridden and the synchronized method modifier isn't inherited, so both versions pass.

Desired Behavior

It's unfortunate that doAnswer() has different semantics between Mockito 4 and 5, but I understand major version bumps imply breaking changes. It's REALLY unfortunate though that Mockito has different semantics depending on whether you use synchronized as a method modifier vs wrap the entire method in a synchronized block.

Would it be possible for the INLINE MockMaker to rewrite synchronized methods to use a synchronized block? This would provide consistent behavior for equivalent syntax as well as consistency with the previous Mockito versions. If not, can we get a spy(Object, MockSettings) overload?

Possibly related: verify with timeout causes deadlock on synchronized methods (#2500)

Environment

Mockito: 5.2.0
JDK: 17.0.6
OS: OSX 13.3.1 aarch64

[TimvdLippe]

@raphw this one is for you 😄

[raphw]

Both problem and solution crossed my mind before, but it's non trivial to solve as methods can be rather convoluted. Maybe I can include a possibility for adding a monitor in advice which already has all the facilities. I'll have a look.