[Guice] Replace CucumberModules.SCENARIO with threadsafe factory method

Fixes an issue where the same SequentialScenarioScope is shared across multiple threads when running in parallel. This causes IllegalStateExceptions to be thrown. CucumberModules.SCENARIO and CucumberScopes.SCENARIO have been deprecated and replaced with a factory methods - CucumberModules.createScenarioModule() and CucumberScopes.createScenario() respectively. Fixes: #1485
cucumber · Oct 19, 2018 · c1beb21 · c1beb21
1 parent 98ca518
commit c1beb21
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 25 deletions.
diff --git a/guice/src/main/java/cucumber/api/guice/CucumberModules.java b/guice/src/main/java/cucumber/api/guice/CucumberModules.java
@@ -1,13 +1,32 @@
 package cucumber.api.guice;
 
 import com.google.inject.Module;
+import cucumber.runtime.java.guice.ScenarioScope;
+import cucumber.runtime.java.guice.ScenarioScoped;
 import cucumber.runtime.java.guice.impl.ScenarioModule;
 
 /**
- * Provides a convenient <code>com.google.inject.Module</code> instance that contains bindings for
- * <code>cucumber.runtime.java.guice.ScenarioScoped</code> annotation and for
- * <code>cucumber.runtime.java.guice.ScenarioScope</code>.
+ * Provides a convenient {@link Module} instance that contains bindings for
+ * {@link ScenarioScoped} annotation and for {@link ScenarioScope}.
  */
 public class CucumberModules {
-    public static final Module SCENARIO = new ScenarioModule(CucumberScopes.SCENARIO);
+    /**
+     * A convenient instance of {@link Module}. Should only be used
+     * in combination with {@link CucumberScopes#SCENARIO}.
+     * <p>
+     * Note that using this in combination with parallel execution results in
+     * undefined behaviour.
+     *
+     * @deprecated please use {@link #createScenarioModule()} instead
+     */
+    @Deprecated
+    public static final Module SCENARIO = createScenarioModule();
+
+    public static Module createScenarioModule() {
+        return new ScenarioModule(CucumberScopes.createScenarioScope());
+    }
+
+    public static Module createScenarioModule(ScenarioScope scenarioScope) {
+        return new ScenarioModule(scenarioScope);
+    }
 }
diff --git a/guice/src/main/java/cucumber/api/guice/CucumberScopes.java b/guice/src/main/java/cucumber/api/guice/CucumberScopes.java
@@ -1,12 +1,39 @@
 package cucumber.api.guice;
 
+import com.google.inject.Module;
 import cucumber.runtime.java.guice.ScenarioScope;
+import cucumber.runtime.java.guice.ScenarioScoped;
 import cucumber.runtime.java.guice.impl.SequentialScenarioScope;
 
 /**
- * Provides a convenient <code>cucumber.runtime.java.guice.ScenarioScope</code> instance for use when declaring bindings
- * in implementations of <code>com.google.inject.Module</code>.
+ * Creates an instance of {@link ScenarioScope} for use when declaring bindings
+ * in implementations of {@link Module}.
+ * <p>
+ * Note that when binding objects to the scenario scope it is recommended to bind
+ * them to the {@link ScenarioScoped} annotation instead. E.g:
+ *
+ * <code>bind(ScenarioScopedObject.class).in(ScenarioScoped.class);</code>
  */
 public class CucumberScopes {
-    public static final ScenarioScope SCENARIO = new SequentialScenarioScope();
+    /**
+     * A convenient instance of {@link ScenarioScope}. Should only be used
+     * in combination with {@link CucumberModules#SCENARIO}.
+     * <p>
+     * Note that using this in combination with parallel execution results in
+     * undefined behaviour.
+     *
+     * @deprecated please use {@link #createScenarioScope()} instead
+     */
+    @Deprecated
+    public static final ScenarioScope SCENARIO = createScenarioScope();
+
+    /**
+     * Creates a new instance of a ScenarioScope.
+     *
+     * @return a new instance of a ScenarioScope.
+     */
+    public static ScenarioScope createScenarioScope() {
+        return new SequentialScenarioScope();
+    }
+
 }
diff --git a/guice/src/main/java/cucumber/api/guice/package.html b/guice/src/main/java/cucumber/api/guice/package.html
@@ -121,7 +121,7 @@ <h3>Using module bindings</h3>
     Guice injector and it's Guice modules. The injector must provide a binding for
     <code>cucumber.runtime.java.guice.ScenarioScope</code>. It should also provide a binding for the
     <code>cucumber.runtime.java.guice.ScenarioScoped</code> annotation if your classes are using the annotation. The
-    easiest way to do this it to use <code>CucumberModules.SCENARIO</code>. For example:
+    easiest way to do this it to use <code>CucumberModules.createScenarioModule()</code>. For example:
 </p>
     <pre>
         import com.google.inject.Guice;
@@ -134,7 +134,7 @@ <h3>Using module bindings</h3>
 
             {@literal @}Override
             public Injector getInjector() {
-                return Guice.createInjector(Stage.PRODUCTION, CucumberModules.SCENARIO, new YourModule());
+                return Guice.createInjector(Stage.PRODUCTION, CucumberModules.createScenarioModule(), new YourModule());
             }
         }
     </pre>

diff --git a/guice/src/test/java/cucumber/runtime/java/guice/impl/GuiceFactoryTest.java b/guice/src/test/java/cucumber/runtime/java/guice/impl/GuiceFactoryTest.java
@@ -8,7 +8,6 @@
 import com.google.inject.Scopes;
 import com.google.inject.Stage;
 import cucumber.api.guice.CucumberModules;
-import cucumber.api.guice.CucumberScopes;
 import cucumber.api.java.ObjectFactory;
 import cucumber.runtime.java.guice.ScenarioScoped;
 import org.junit.After;
@@ -65,14 +64,14 @@ static class UnscopedClass {}
 
     @Test
     public void shouldGiveNewInstancesOfUnscopedClassWithinAScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromSameScenario = getInstancesFromSameScenario(factory, UnscopedClass.class);
         assertThat(instancesFromSameScenario, elementsAreAllUnique());
     }
 
     @Test
     public void shouldGiveNewInstanceOfUnscopedClassForEachScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromDifferentScenarios = getInstancesFromDifferentScenarios(factory, UnscopedClass.class);
         assertThat(instancesFromDifferentScenarios, elementsAreAllUnique());
     }
@@ -81,14 +80,14 @@ public void shouldGiveNewInstanceOfUnscopedClassForEachScenario() {
 
     @Test
     public void shouldGiveTheSameInstanceOfAnnotatedScenarioScopedClassWithinAScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromSameScenario = getInstancesFromSameScenario(factory, AnnotatedScenarioScopedClass.class);
         assertThat(instancesFromSameScenario, elementsAreAllEqual());
     }
 
     @Test
     public void shouldGiveNewInstanceOfAnnotatedScenarioScopedClassForEachScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromDifferentScenarios = getInstancesFromDifferentScenarios(factory, AnnotatedScenarioScopedClass.class);
         assertThat(instancesFromDifferentScenarios, elementsAreAllUnique());
     }
@@ -97,14 +96,14 @@ public void shouldGiveNewInstanceOfAnnotatedScenarioScopedClassForEachScenario()
 
     @Test
     public void shouldGiveTheSameInstanceOfAnnotatedSingletonClassWithinAScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromSameScenario = getInstancesFromSameScenario(factory, AnnotatedSingletonClass.class);
         assertThat(instancesFromSameScenario, elementsAreAllEqual());
     }
 
     @Test
     public void shouldGiveTheSameInstanceOfAnnotatedSingletonClassForEachScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule()));
         instancesFromDifferentScenarios = getInstancesFromDifferentScenarios(factory, AnnotatedSingletonClass.class);
         assertThat(instancesFromDifferentScenarios, elementsAreAllEqual());
     }
@@ -113,13 +112,13 @@ static class BoundScenarioScopedClass {}
 
     final AbstractModule boundScenarioScopedClassModule = new AbstractModule() {
         @Override protected void configure() {
-            bind(BoundScenarioScopedClass.class).in(CucumberScopes.SCENARIO);
+            bind(BoundScenarioScopedClass.class).in(ScenarioScoped.class);
         }
     };
 
     @Test
     public void shouldGiveTheSameInstanceOfBoundScenarioScopedClassWithinAScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO, boundScenarioScopedClassModule));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule(), boundScenarioScopedClassModule));
         instancesFromSameScenario = getInstancesFromSameScenario(factory, BoundScenarioScopedClass.class);
         assertThat(instancesFromSameScenario, elementsAreAllEqual());
     }
@@ -148,17 +147,17 @@ public void shouldGiveTheSameInstanceOfBoundSingletonClassWithinAScenario() {
 
     @Test
     public void shouldGiveTheSameInstanceOfBoundSingletonClassForEachScenario() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO, boundSingletonClassModule));
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule(), boundSingletonClassModule));
         instancesFromDifferentScenarios = getInstancesFromDifferentScenarios(factory, BoundSingletonClass.class);
         assertThat(instancesFromDifferentScenarios, elementsAreAllEqual());
     }
 
     @Test
     public void shouldGiveNewInstanceOfAnnotatedSingletonClassForEachScenarioWhenOverridingModuleBindingIsScenarioScope() {
-        factory = new GuiceFactory(injector(CucumberModules.SCENARIO, new AbstractModule() {
+        factory = new GuiceFactory(injector(CucumberModules.createScenarioModule(), new AbstractModule() {
             @Override
             protected void configure() {
-                bind(AnnotatedSingletonClass.class).in(CucumberScopes.SCENARIO);
+                bind(AnnotatedSingletonClass.class).in(ScenarioScoped.class);
             }
         }));
         instancesFromDifferentScenarios = getInstancesFromDifferentScenarios(factory, AnnotatedSingletonClass.class);
@@ -202,4 +201,4 @@ private <E> List<E> getInstancesFromDifferentScenarios(ObjectFactory factory, Cl
         return Arrays.asList(o1, o2, o3);
     }
 
-}
+}
diff --git a/guice/src/test/java/cucumber/runtime/java/guice/integration/YourInjectorSource.java b/guice/src/test/java/cucumber/runtime/java/guice/integration/YourInjectorSource.java
@@ -4,12 +4,14 @@
 import com.google.inject.Injector;
 import com.google.inject.Stage;
 import cucumber.api.guice.CucumberModules;
+import cucumber.api.guice.CucumberScopes;
 import cucumber.runtime.java.guice.InjectorSource;
+import cucumber.runtime.java.guice.ScenarioScope;
 
 public class YourInjectorSource implements InjectorSource {
 
     @Override
     public Injector getInjector() {
-        return Guice.createInjector(Stage.PRODUCTION, CucumberModules.SCENARIO, new YourModule());
+        return Guice.createInjector(Stage.PRODUCTION, CucumberModules.createScenarioModule(), new YourModule());
     }
 }
diff --git a/guice/src/test/java/cucumber/runtime/java/guice/integration/YourModule.java b/guice/src/test/java/cucumber/runtime/java/guice/integration/YourModule.java
@@ -3,13 +3,14 @@
 import com.google.inject.AbstractModule;
 import com.google.inject.Scopes;
 import cucumber.api.guice.CucumberScopes;
+import cucumber.runtime.java.guice.ScenarioScoped;
 
 public class YourModule extends AbstractModule {
 
     @Override
     protected void configure() {
         // UnScopedObject is implicitly bound without scope
-        bind(ScenarioScopedObject.class).in(CucumberScopes.SCENARIO);
+        bind(ScenarioScopedObject.class).in(ScenarioScoped.class);
         bind(SingletonObject.class).in(Scopes.SINGLETON);
     }
-}
+}