Properly fix possible deadlock, when blocking in mock response generators and fix fallout of #1885 (#583, #1899)

docs/release_notes.adoc

@@ -5,6 +5,26 @@ include::include.adoc[]
 
 == 2.4 (tbd)
 
+=== Breaking Changes
+
+* _Most users will probably be unaffected by this change as it only becomes relevant in a multithreaded situation where multiple threads do interaction invocations that care about shared state._ +
+  Calculated responses for interactions (`>> { ... }`) previously were all executed synchronized on the respective mock controller instance, so could safely mutate shared state to a certain degree, even if the invocations were happening
+  on different threads.
+  This also caused that one response calculation could not wait on something happening in another response calculation, as they were all executed sequentially due to the synchronization.
+  Starting with this release, the response calculations are no longer happening synchronized.
+  If you depend on shared state in such calculations, you now have to make sure yourself, that this is done in a thread-safe manner.
+  spockPull:1910[]
+
+* _This should not affect most users, only if you were subclassing `SingleResponseGenerator` and doing unusual things._ +
+  `SingleResponseGenerator#isAtEndOfCycle` is now `final` and `SingleResponseGenerator#doRespond` is now `protected`.
+  When subclassing `SingleResponseGenerator` it does not make sense to override the first method, and it does not make sense to call the second method from somewhere else.
+  spockPull:1910[]
+
+=== Misc
+
+* Properly fix possible deadlock, when blocking in mock response generators and fix fallout of spockPull:1885[]
+** This actually fixes the issues: spockIssue:583[], spockIssue:1882[], spockIssue:1899[]
+
 == 2.4-M2 (2024-02-26)
 
 === Highlights
@@ -70,8 +90,8 @@ include::include.adoc[]
 * Fix exception when configured `baseDir` was not existing, now `@TempDir` will create the baseDir directory if it is missing.
 * Fix bad error message for collection conditions, when one of the operands is `null`
 * Fix docs about initializer method interceptors spockPull:1666[]
-* Fix possible deadlock, when blocking in mock response generators spockPull:1885[]
-** This fixes the issues: spockIssue:583[], spockIssue:1882[]
+* [.line-through]#Fix possible deadlock, when blocking in mock response generators spockPull:1885[]#
+** [.line-through]#This fixes the issues: spockIssue:583[], spockIssue:1882[]#
 * Fix SpringSpy not working with `DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD` spockPull:1869[]
 * Fix null handling for collection conditions spockPull:1858[]
 * Fix interceptor contexts spockPull:1676[]

spock-core/src/main/java/org/spockframework/mock/CallRealMethodResponse.java

@@ -15,18 +15,22 @@
 package org.spockframework.mock;
 
 import org.spockframework.util.Beta;
+import org.spockframework.util.ThreadSafe;
+
+import java.util.function.Supplier;
 
 /**
  * A response strategy that delegates method calls to the real object underlying the mock (if any).
  */
 @Beta
+@ThreadSafe
 public class CallRealMethodResponse implements IDefaultResponse {
   public static final CallRealMethodResponse INSTANCE = new CallRealMethodResponse();
 
   private CallRealMethodResponse() {}
 
   @Override
-  public Object respond(IMockInvocation invocation) {
-    return invocation.callRealMethod();
+  public Supplier<Object> getResponseSupplier(IMockInvocation invocation) {
+    return invocation::callRealMethod;
   }
 }

spock-core/src/main/java/org/spockframework/mock/DefaultCompareToInteraction.java

@@ -1,5 +1,7 @@
 package org.spockframework.mock;
 
+import java.util.function.Supplier;
+
 public class DefaultCompareToInteraction extends DefaultInteraction {
   public static final DefaultCompareToInteraction INSTANCE = new DefaultCompareToInteraction();
 
@@ -21,7 +23,7 @@ public boolean matches(IMockInvocation invocation) {
   }
 
   @Override
-  public Object accept(IMockInvocation invocation) {
-    return Integer.compare(System.identityHashCode(invocation.getMockObject().getInstance()), System.identityHashCode(invocation.getArguments().get(0)));
+  public Supplier<Object> accept(IMockInvocation invocation) {
+    return () -> Integer.compare(System.identityHashCode(invocation.getMockObject().getInstance()), System.identityHashCode(invocation.getArguments().get(0)));
   }
 }

spock-core/src/main/java/org/spockframework/mock/DefaultEqualsInteraction.java

@@ -13,6 +13,8 @@
  */
 package org.spockframework.mock;
 
+import java.util.function.Supplier;
+
 class DefaultEqualsInteraction extends DefaultInteraction {
   public static final DefaultEqualsInteraction INSTANCE = new DefaultEqualsInteraction();
 
@@ -33,7 +35,7 @@ public boolean matches(IMockInvocation invocation) {
   }
 
   @Override
-  public Object accept(IMockInvocation invocation) {
-    return invocation.getMockObject().getInstance() == invocation.getArguments().get(0);
+  public Supplier<Object> accept(IMockInvocation invocation) {
+    return () -> invocation.getMockObject().getInstance() == invocation.getArguments().get(0);
   }
 }

spock-core/src/main/java/org/spockframework/mock/DefaultFinalizeInteraction.java

@@ -13,6 +13,8 @@
  */
 package org.spockframework.mock;
 
+import java.util.function.Supplier;
+
 public class DefaultFinalizeInteraction extends DefaultInteraction {
   public static final DefaultFinalizeInteraction INSTANCE = new DefaultFinalizeInteraction();
 
@@ -31,7 +33,7 @@ public boolean matches(IMockInvocation invocation) {
   }
 
   @Override
-  public Object accept(IMockInvocation invocation) {
-    return null;
+  public Supplier<Object> accept(IMockInvocation invocation) {
+    return () -> null;
   }
 }

spock-core/src/main/java/org/spockframework/mock/DefaultHashCodeInteraction.java

@@ -13,6 +13,8 @@
  */
 package org.spockframework.mock;
 
+import java.util.function.Supplier;
+
 class DefaultHashCodeInteraction extends DefaultInteraction {
   public static final DefaultHashCodeInteraction INSTANCE = new DefaultHashCodeInteraction();
 
@@ -30,7 +32,7 @@ public boolean matches(IMockInvocation invocation) {
   }
 
   @Override
-  public Object accept(IMockInvocation invocation) {
-    return System.identityHashCode(invocation.getMockObject().getInstance());
+  public Supplier<Object> accept(IMockInvocation invocation) {
+    return () -> System.identityHashCode(invocation.getMockObject().getInstance());
   }
 }

spock-core/src/main/java/org/spockframework/mock/DefaultToStringInteraction.java

@@ -14,6 +14,8 @@
 
 package org.spockframework.mock;
 
+import java.util.function.Supplier;
+
 class DefaultToStringInteraction extends DefaultInteraction {
   public static final DefaultToStringInteraction INSTANCE = new DefaultToStringInteraction();
 
@@ -31,19 +33,21 @@ public boolean matches(IMockInvocation invocation) {
   }
 
   @Override
-  public Object accept(IMockInvocation invocation) {
-    StringBuilder builder = new StringBuilder();
-    builder.append("Mock for type '");
-    builder.append(invocation.getMockObject().getType().getSimpleName());
-    builder.append("'");
-
-    String name = invocation.getMockObject().getName();
-    if (name != null) {
-      builder.append(" named '");
-      builder.append(name);
+  public Supplier<Object> accept(IMockInvocation invocation) {
+    return () -> {
+      StringBuilder builder = new StringBuilder();
+      builder.append("Mock for type '");
+      builder.append(invocation.getMockObject().getType().getSimpleName());
       builder.append("'");
-    }
 
-    return builder.toString();
+      String name = invocation.getMockObject().getName();
+      if (name != null) {
+        builder.append(" named '");
+        builder.append(name);
+        builder.append("'");
+      }
+
+      return builder.toString();
+    };
   }
 }

spock-core/src/main/java/org/spockframework/mock/EmptyOrDummyResponse.java

@@ -15,6 +15,7 @@
 package org.spockframework.mock;
 
 import org.spockframework.util.ReflectionUtil;
+import org.spockframework.util.ThreadSafe;
 import spock.lang.Specification;
 
 import java.lang.reflect.*;
@@ -31,6 +32,7 @@
  * A response strategy that returns zero, an "empty" object, or a "dummy" object,
  * depending on the method's declared return type.
  */
+@ThreadSafe
 public class EmptyOrDummyResponse implements IDefaultResponse {
   public static final EmptyOrDummyResponse INSTANCE = new EmptyOrDummyResponse();
 
@@ -40,7 +42,7 @@ private EmptyOrDummyResponse() {}
   @SuppressWarnings("rawtypes")
   public Object respond(IMockInvocation invocation) {
     IMockInteraction interaction = DefaultJavaLangObjectInteractions.INSTANCE.match(invocation);
-    if (interaction != null) return interaction.accept(invocation);
+    if (interaction != null) return interaction.accept(invocation).get();
 
     Class<?> returnType = invocation.getMethod().getReturnType();
 

spock-core/src/main/java/org/spockframework/mock/IChainableResponseGenerator.java

@@ -22,5 +22,21 @@
  * @author Peter Niederwieser
  */
 public interface IChainableResponseGenerator extends IResponseGenerator {
+  /**
+   * Whether this chainable response generator is at the end of its cycle. If this method returns {@code true},
+   * the next response generator in the chain is used if one is available. If this method returns {@code false} or
+   * there is no next response generator in the chain, the {@link #getResponseSupplier(IMockInvocation)} method will
+   * be called further on for all matched invocations.
+   *
+   * <p>The {@code getResponseSupplier} method and this method will be called under a common lock to make sure code
+   * in {@code getResponseSupplier} can update state like setting a boolean or advancing an iterator, that is then
+   * checked in the implementation of this method. Such state updates should therefore be done within
+   * {@code getResponseSupplier} directly and not within the returned {@code Supplier} or in
+   * {@link #respond(IMockInvocation)}, otherwise the iteration matching algorithm will break. All other logic
+   * should preferably be done within the {@code Supplier}, especially if the code could deadlock like closures
+   * waiting for a common condition.
+   *
+   * @return Whether this chainable response generator is at the end of its cycle
+   */
   boolean isAtEndOfCycle();
 }

spock-core/src/main/java/org/spockframework/mock/IMockInteraction.java

@@ -19,6 +19,7 @@
 import org.spockframework.util.Nullable;
 
 import java.util.List;
+import java.util.function.Supplier;
 
 /**
  * An anticipated interaction between the SUT and one or more mock objects.
@@ -34,8 +35,7 @@ public interface IMockInteraction {
 
   boolean matches(IMockInvocation invocation);
 
-  @Nullable
-  Object accept(IMockInvocation invocation);
+  Supplier<Object> accept(IMockInvocation invocation);
 
   List<IMockInvocation> getAcceptedInvocations();
 

spock-core/src/main/java/org/spockframework/mock/IResponseGenerator.java

@@ -15,11 +15,55 @@
 package org.spockframework.mock;
 
 import org.spockframework.util.Beta;
+import org.spockframework.util.Nullable;
+
+import java.util.function.Supplier;
 
 /**
  * Generates responses to mock invocations.
  */
 @Beta
 public interface IResponseGenerator {
-  Object respond(IMockInvocation invocation);
+  /**
+   * Returns the response to be made for the given invocation. It is preferable to call
+   * {@link #getResponseSupplier(IMockInvocation)} instead, as that method can separate immediate actions that might
+   * need to be done under some lock from actions that can be done later in the returned supplier.
+   *
+   * <p>The default implementation of this method uses {@code getResponseSupplier}, the default implementation of which
+   * uses this method. Implementations of this interface should override exactly one of these two methods. If they
+   * override both, the result could be different depending on which method is called, if they override none,
+   * any call will result in a {@code StackOverflowError}. Calls to the two methods must always behave consistent,
+   * and the easiest way to achieve that is to override only one of them.
+   *
+   * <p>If you implement this interface and do not need to update shared state immediately like when implementing
+   * {@link IChainableResponseGenerator}, you can just override this method. If you need the separation of immediate
+   * actions done under some lock and actions that can be done delayed, better override {@code getResponseSupplier}.
+   *
+   * @param invocation The invocation to generate a response for
+   * @return The generated response
+   */
+  @Nullable
+  default Object respond(IMockInvocation invocation) {
+    return getResponseSupplier(invocation).get();
+  }
+
+  /**
+   * Returns a supplier with the response to be made for the given invocation.
+   *
+   * <p>The default implementation of this method uses {@link #respond(IMockInvocation)}, the default implementation
+   * of which uses this method. Implementations of this interface should override exactly one of these two methods.
+   * If they override both, the result could be different depending on which method is called, if they override none,
+   * any call will result in a {@code StackOverflowError}. Calls to the two methods must always behave consistent,
+   * and the easiest way to achieve that is to override only one of them.
+   *
+   * <p>If you implement this interface and do need to update shared state immediately like when implementing
+   * {@link IChainableResponseGenerator}, you should prefer overriding this method. If not, then you might consider
+   * just overriding {@code respond} instead.
+   *
+   * @param invocation The invocation to generate a response for
+   * @return A supplier that provides the generated response
+   */
+  default Supplier<Object> getResponseSupplier(IMockInvocation invocation) {
+    return () -> respond(invocation);
+  }
 }

spock-core/src/main/java/org/spockframework/mock/ZeroOrNullResponse.java

@@ -20,6 +20,7 @@
  * A response strategy that returns zero, false, or null, depending on the method's return type.
  */
 @Beta
+@ThreadSafe
 public class ZeroOrNullResponse implements IDefaultResponse {
   public static final ZeroOrNullResponse INSTANCE = new ZeroOrNullResponse();
 
@@ -28,7 +29,7 @@ private ZeroOrNullResponse() {}
   @Override
   public Object respond(IMockInvocation invocation) {
     IMockInteraction interaction = DefaultJavaLangObjectInteractions.INSTANCE.match(invocation);
-    if (interaction != null) return interaction.accept(invocation);
+    if (interaction != null) return interaction.accept(invocation).get();
 
     Class<?> returnType = invocation.getMethod().getReturnType();
     return ReflectionUtil.getDefaultValue(returnType);

spock-core/src/main/java/org/spockframework/mock/response/CodeResponseGenerator.java

@@ -20,11 +20,15 @@
 import org.spockframework.runtime.GroovyRuntimeUtil;
 
 import groovy.lang.Closure;
+import org.spockframework.util.ThreadSafe;
+
+import static org.spockframework.util.ObjectUtil.uncheckedCast;
 
 /**
  *
  * @author Peter Niederwieser
  */
+@ThreadSafe
 public class CodeResponseGenerator extends SingleResponseGenerator {
   private final Closure code;
 
@@ -49,6 +53,7 @@ private Object invokeClosure(IMockInvocation invocation) {
       return GroovyRuntimeUtil.invokeClosure(code, invocation);
     }
 
+    Closure<?> code = uncheckedCast(this.code.clone());
     code.setDelegate(invocation);
     code.setResolveStrategy(Closure.DELEGATE_FIRST);
     return GroovyRuntimeUtil.invokeClosure(code, invocation.getArguments());

spock-core/src/main/java/org/spockframework/mock/response/ConstantResponseGenerator.java

@@ -18,11 +18,13 @@
 
 import org.spockframework.mock.IMockInvocation;
 import org.spockframework.runtime.GroovyRuntimeUtil;
+import org.spockframework.util.ThreadSafe;
 
 /**
  *
  * @author Peter Niederwieser
  */
+@ThreadSafe
 public class ConstantResponseGenerator extends SingleResponseGenerator {
   private final Object constant;
 

spock-core/src/main/java/org/spockframework/mock/response/DefaultResponseGenerator.java

@@ -17,10 +17,14 @@
 package org.spockframework.mock.response;
 
 import org.spockframework.mock.*;
+import org.spockframework.util.ThreadSafe;
 
+import java.util.function.Supplier;
+
+@ThreadSafe
 public class DefaultResponseGenerator implements IResponseGenerator {
   @Override
-  public Object respond(IMockInvocation invocation) {
-    return invocation.getMockObject().getDefaultResponse().respond(invocation);
+  public Supplier<Object> getResponseSupplier(IMockInvocation invocation) {
+    return invocation.getMockObject().getDefaultResponse().getResponseSupplier(invocation);
   }
 }