From c1beb21b4490bb54e692f46c8afc4fe3f9b8eff8 Mon Sep 17 00:00:00 2001 From: James Bennett Date: Thu, 18 Oct 2018 19:50:51 +0100 Subject: [PATCH] [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/api/guice/CucumberModules.java | 27 ++++++++++++--- .../cucumber/api/guice/CucumberScopes.java | 33 +++++++++++++++++-- .../main/java/cucumber/api/guice/package.html | 4 +-- .../java/guice/impl/GuiceFactoryTest.java | 25 +++++++------- .../guice/integration/YourInjectorSource.java | 4 ++- .../java/guice/integration/YourModule.java | 5 +-- 6 files changed, 73 insertions(+), 25 deletions(-) diff --git a/guice/src/main/java/cucumber/api/guice/CucumberModules.java b/guice/src/main/java/cucumber/api/guice/CucumberModules.java index 8a78912556..424ae6a875 100644 --- 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 com.google.inject.Module instance that contains bindings for - * cucumber.runtime.java.guice.ScenarioScoped annotation and for - * cucumber.runtime.java.guice.ScenarioScope. + * 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}. + *

+ * 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 index b9dac00415..fafe7c6ba5 100644 --- 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 cucumber.runtime.java.guice.ScenarioScope instance for use when declaring bindings - * in implementations of com.google.inject.Module. + * Creates an instance of {@link ScenarioScope} for use when declaring bindings + * in implementations of {@link Module}. + *

+ * Note that when binding objects to the scenario scope it is recommended to bind + * them to the {@link ScenarioScoped} annotation instead. E.g: + * + * bind(ScenarioScopedObject.class).in(ScenarioScoped.class); */ 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}. + *

+ * 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 index 317aaa8a9c..163e90fe92 100644 --- a/guice/src/main/java/cucumber/api/guice/package.html +++ b/guice/src/main/java/cucumber/api/guice/package.html @@ -121,7 +121,7 @@

Using module bindings

Guice injector and it's Guice modules. The injector must provide a binding for cucumber.runtime.java.guice.ScenarioScope. It should also provide a binding for the cucumber.runtime.java.guice.ScenarioScoped annotation if your classes are using the annotation. The - easiest way to do this it to use CucumberModules.SCENARIO. For example: + easiest way to do this it to use CucumberModules.createScenarioModule(). For example: 

         import com.google.inject.Guice;
@@ -134,7 +134,7 @@ 
Using module bindings

 
             {@literal @}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/impl/GuiceFactoryTest.java b/guice/src/test/java/cucumber/runtime/java/guice/impl/GuiceFactoryTest.java index 11ada40622..90718db497 100644 --- 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 List getInstancesFromDifferentScenarios(ObjectFactory factory, Cl return Arrays.asList(o1, o2, o3); } -} \ No newline at end of file +} 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 index d6be328a3a..39a0e2190f 100644 --- 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 index 870ee22fea..8a8b631585 100644 --- 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); } -} \ No newline at end of file +}