Streamline Guice Module creation in concurrency

Closes #3048 #3050

CHANGES.txt

@@ -1,5 +1,7 @@
 Current
 7.10.0
+Fixed: GITHUB-3048: ConcurrentModificationException when injecting values (Krishnan Mahadevan)
+Fixed: GITHUB-3050: Race condition when creating Guice Modules (Krishnan Mahadevan)
 Fixed: GITHUB-3059: Support the ability to inject custom listener factory (Krishnan Mahadevan)
 Fixed: GITHUB-3045: IDataProviderListener - beforeDataProviderExecution and afterDataProviderExecution are called twice in special setup (Krishnan Mahadevan)
 Fixed: GITHUB-3038: java.lang.IllegalStateException: Results per method should NOT have been empty (Krishnan Mahadevan)

testng-core/src/main/java/org/testng/internal/objects/GuiceBasedObjectDispenser.java

@@ -1,6 +1,7 @@
 package org.testng.internal.objects;
 
 import com.google.inject.Injector;
+import java.util.concurrent.locks.ReentrantLock;
 import org.testng.ITestContext;
 import org.testng.annotations.Guice;
 import org.testng.internal.annotations.AnnotationHelper;
@@ -12,6 +13,7 @@
 class GuiceBasedObjectDispenser implements IObjectDispenser {
 
   private IObjectDispenser dispenser;
+  private static final ReentrantLock lock = new ReentrantLock();
 
   @Override
   public void setNextDispenser(IObjectDispenser dispenser) {
@@ -20,43 +22,50 @@ public void setNextDispenser(IObjectDispenser dispenser) {
 
   @Override
   public Object dispense(CreationAttributes attributes) {
-    if (attributes.getBasicAttributes() != null) {
-      BasicAttributes sa = attributes.getBasicAttributes();
-      Class<?> cls =
-          sa.getTestClass() == null ? sa.getRawClass() : sa.getTestClass().getRealClass();
-      if (cannotDispense(cls)) {
-        return this.dispenser.dispense(attributes);
-      }
-      ITestContext ctx = attributes.getContext();
-      GuiceContext suiteCtx = attributes.getSuiteContext();
-      GuiceHelper helper;
-      // TODO: remove unused entries from helpers
-      if (ctx == null) {
-        helper = new GuiceHelper(suiteCtx);
-      } else {
-        helper = (GuiceHelper) ctx.getAttribute(GUICE_HELPER);
-        if (helper == null) {
-          helper = new GuiceHelper(ctx);
-          ctx.setAttribute(GUICE_HELPER, helper);
-        }
-      }
-      Injector injector;
-      if (ctx == null) {
-        injector = helper.getInjector(cls, suiteCtx.getInjectorFactory());
-      } else {
-        injector = helper.getInjector(cls, ctx.getInjectorFactory());
-      }
-      if (injector == null) {
-        return null;
-      }
-      if (sa.getRawClass() == null) {
-        return injector.getInstance(sa.getTestClass().getRealClass());
-      } else {
-        return injector.getInstance(sa.getRawClass());
+    if (attributes.getBasicAttributes() == null) {
+      // We don't have the ability to process object creation with elaborate attributes
+      return this.dispenser.dispense(attributes);
+    }
+    try {
+      lock.lock();
+      return dispenseObject(attributes);
+    } finally {
+      lock.unlock();
+    }
+  }
+
+  private Object dispenseObject(CreationAttributes attributes) {
+    BasicAttributes sa = attributes.getBasicAttributes();
+    Class<?> cls = sa.getTestClass() == null ? sa.getRawClass() : sa.getTestClass().getRealClass();
+    if (cannotDispense(cls)) {
+      return this.dispenser.dispense(attributes);
+    }
+    ITestContext ctx = attributes.getContext();
+    GuiceContext suiteCtx = attributes.getSuiteContext();
+    GuiceHelper helper;
+    // TODO: remove unused entries from helpers
+    if (ctx == null) {
+      helper = new GuiceHelper(suiteCtx);
+    } else {
+      helper = (GuiceHelper) ctx.getAttribute(GUICE_HELPER);
+      if (helper == null) {
+        helper = new GuiceHelper(ctx);
+        ctx.setAttribute(GUICE_HELPER, helper);
       }
     }
-    // We dont have the ability to process object creation with elaborate attributes
-    return this.dispenser.dispense(attributes);
+    Injector injector;
+    if (ctx == null) {
+      injector = helper.getInjector(cls, suiteCtx.getInjectorFactory());
+    } else {
+      injector = helper.getInjector(cls, ctx.getInjectorFactory());
+    }
+    if (injector == null) {
+      return null;
+    }
+    if (sa.getRawClass() == null) {
+      return injector.getInstance(sa.getTestClass().getRealClass());
+    }
+    return injector.getInstance(sa.getRawClass());
   }
 
   private static boolean cannotDispense(Class<?> clazz) {

testng-core/src/test/java/test/guice/GuiceTest.java

@@ -6,11 +6,14 @@
 import com.google.inject.Injector;
 import com.google.inject.Module;
 import com.google.inject.Stage;
+import java.util.List;
 import org.jetbrains.annotations.Nullable;
 import org.testng.IInjectorFactory;
 import org.testng.TestNG;
 import org.testng.annotations.Test;
+import org.testng.xml.XmlPackage;
 import org.testng.xml.XmlSuite;
+import org.testng.xml.XmlTest;
 import test.SimpleBaseTest;
 import test.guice.issue2343.Person;
 import test.guice.issue2343.SampleA;
@@ -26,6 +29,7 @@
 import test.guice.issue2570.GuicePoweredConstructorInjectedRetryForDPTest;
 import test.guice.issue2570.GuicePoweredSetterInjectedRetry;
 import test.guice.issue2570.SampleTestClass;
+import test.guice.issue3050.EvidenceRetryAnalyzer;
 
 public class GuiceTest extends SimpleBaseTest {
 
@@ -108,4 +112,17 @@ public void ensureRetryAnalyzersAreGuiceAware() {
         .withFailMessage("There should have been 2 retry analyser instances created by Guice")
         .isEqualTo(2);
   }
+
+  @Test(description = "GITHUB-3050")
+  public void ensureNoDuplicateGuiceModulesAreCreated() {
+    TestNG testng = create();
+    XmlSuite xmlSuite = createXmlSuite("evidence_suite");
+    xmlSuite.setParallel(XmlSuite.ParallelMode.CLASSES);
+    xmlSuite.setThreadCount(7);
+    XmlTest xmlTest = createXmlTest(xmlSuite, "evidence_test");
+    xmlTest.setXmlPackages(List.of(new XmlPackage("test.guice.issue3050.*")));
+    testng.setXmlSuites(List.of(xmlSuite));
+    testng.run();
+    assertThat(EvidenceRetryAnalyzer.hasOnlyOneUuid()).isTrue();
+  }
 }

testng-core/src/test/java/test/guice/issue3050/EvidenceModule.java

@@ -0,0 +1,15 @@
+package test.guice.issue3050;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.Provides;
+import com.google.inject.Singleton;
+import java.util.UUID;
+
+public class EvidenceModule extends AbstractModule {
+
+  @Provides
+  @Singleton
+  private UUID randomUUID() {
+    return UUID.randomUUID();
+  }
+}

testng-core/src/test/java/test/guice/issue3050/EvidenceRetryAnalyzer.java

@@ -0,0 +1,27 @@
+package test.guice.issue3050;
+
+import com.google.inject.Inject;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import org.testng.IRetryAnalyzer;
+import org.testng.ITestResult;
+import org.testng.annotations.Guice;
+
+@Guice(modules = EvidenceModule.class)
+public class EvidenceRetryAnalyzer implements IRetryAnalyzer {
+
+  public static Set<UUID> uuids = ConcurrentHashMap.newKeySet();
+
+  public static boolean hasOnlyOneUuid() {
+    return uuids.size() == 1;
+  }
+
+  @Inject private UUID injectedUUID;
+
+  @Override
+  public boolean retry(ITestResult result) {
+    uuids.add(injectedUUID);
+    return false;
+  }
+}

testng-core/src/test/java/test/guice/issue3050/EvidenceTestOneSample.java

@@ -0,0 +1,31 @@
+package test.guice.issue3050;
+
+import static org.testng.Assert.fail;
+
+import org.testng.ITestContext;
+import org.testng.ITestNGMethod;
+import org.testng.Reporter;
+import org.testng.annotations.BeforeSuite;
+import org.testng.annotations.Test;
+
+@Test
+public class EvidenceTestOneSample {
+
+  @BeforeSuite(alwaysRun = true)
+  public void suiteSetup() {
+    ITestContext context = Reporter.getCurrentTestResult().getTestContext();
+    for (ITestNGMethod method : context.getAllTestMethods()) {
+      method.setRetryAnalyzerClass(EvidenceRetryAnalyzer.class);
+    }
+  }
+
+  @Test
+  public void testOne() {
+    fail();
+  }
+
+  @Test
+  public void testTwo() {
+    fail();
+  }
+}

testng-core/src/test/java/test/guice/issue3050/EvidenceTestThreeSample.java

@@ -0,0 +1,31 @@
+package test.guice.issue3050;
+
+import static org.testng.Assert.fail;
+
+import org.testng.ITestContext;
+import org.testng.ITestNGMethod;
+import org.testng.Reporter;
+import org.testng.annotations.BeforeSuite;
+import org.testng.annotations.Test;
+
+@Test
+public class EvidenceTestThreeSample {
+
+  @BeforeSuite(alwaysRun = true)
+  public void suiteSetup() {
+    ITestContext context = Reporter.getCurrentTestResult().getTestContext();
+    for (ITestNGMethod method : context.getAllTestMethods()) {
+      method.setRetryAnalyzerClass(EvidenceRetryAnalyzer.class);
+    }
+  }
+
+  @Test
+  public void testOne() {
+    fail();
+  }
+
+  @Test
+  public void testTwo() {
+    fail();
+  }
+}

testng-core/src/test/java/test/guice/issue3050/EvidenceTestTwoSample.java

@@ -0,0 +1,31 @@
+package test.guice.issue3050;
+
+import static org.testng.Assert.fail;
+
+import org.testng.ITestContext;
+import org.testng.ITestNGMethod;
+import org.testng.Reporter;
+import org.testng.annotations.BeforeSuite;
+import org.testng.annotations.Test;
+
+@Test
+public class EvidenceTestTwoSample {
+
+  @BeforeSuite(alwaysRun = true)
+  public void suiteSetup() {
+    ITestContext context = Reporter.getCurrentTestResult().getTestContext();
+    for (ITestNGMethod method : context.getAllTestMethods()) {
+      method.setRetryAnalyzerClass(EvidenceRetryAnalyzer.class);
+    }
+  }
+
+  @Test
+  public void testOne() {
+    fail();
+  }
+
+  @Test
+  public void testTwo() {
+    fail();
+  }
+}