From b45b60a7bd64f1b715679e1ce4df6a7752149d18 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 17:31:30 +0300 Subject: [PATCH 1/4] Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init SQLUtil tested "instanceof Integer" twice, the second branch unreachable. ContractITCase dereferenced a factory that was always null; it now uses TestNG's ObjectFactoryImpl, and the unused nested ContractTestFactory goes. BatchRemoteCache synchronised on an interned string literal, a monitor shared with any other code using the same literal; it locks a private object now. The double-checked lazy initialisation of WebSocketConnectionGroup.operationContext, ScriptedConfiguration.groovyScriptEngine and TstStatefulConnectorConfig.executorService reads a non-volatile field outside the lock; the fields are volatile now. FrameworkUtil's version getter is synchronised like its setter, and the class-loader overload, which reads a resource rather than the field, is readFrameworkVersion. --- .../org/identityconnectors/dbcommon/SQLUtil.java | 2 -- .../scriptedcommon/ScriptedConfiguration.java | 2 +- .../contract/test/ContractITCase.java | 15 ++------------- .../remote/rpc/WebSocketConnectionGroup.java | 2 +- .../framework/common/FrameworkUtil.java | 5 +++-- .../framework/common/FrameworkUtilTests.java | 5 +++-- .../testconnector/BatchRemoteCache.java | 3 ++- .../testconnector/TstStatefulConnectorConfig.java | 2 +- 8 files changed, 13 insertions(+), 23 deletions(-) diff --git a/OpenICF-dbcommon/src/main/java/org/identityconnectors/dbcommon/SQLUtil.java b/OpenICF-dbcommon/src/main/java/org/identityconnectors/dbcommon/SQLUtil.java index 8b9d5d35f..08de42bab 100644 --- a/OpenICF-dbcommon/src/main/java/org/identityconnectors/dbcommon/SQLUtil.java +++ b/OpenICF-dbcommon/src/main/java/org/identityconnectors/dbcommon/SQLUtil.java @@ -822,8 +822,6 @@ public static void setSQLParam(final PreparedStatement stmt, final int idx, SQLP stmt.setLong(idx, ((BigInteger) val).longValue()); } else if (val instanceof Byte) { stmt.setByte(idx, (Byte) val); - } else if (val instanceof Integer) { - stmt.setInt(idx, (Integer) val); } else if (val instanceof InputStream) { stmt.setBinaryStream(idx, (InputStream) val, 10000); } else if (val instanceof Blob) { diff --git a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java index a918990dd..f8aa4443f 100644 --- a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java +++ b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java @@ -782,7 +782,7 @@ protected Log getLogger(final Class clazz) { return logger; } - private GroovyScriptEngine groovyScriptEngine = null; + private volatile GroovyScriptEngine groovyScriptEngine = null; /** * Synchronised for the whole initialisation, not double-checked: the diff --git a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java index 398cef046..0002b15d6 100644 --- a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java +++ b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java @@ -20,6 +20,7 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.contract.test; @@ -32,8 +33,6 @@ import org.identityconnectors.common.StringUtil; import org.identityconnectors.contract.data.DataProvider; -import org.identityconnectors.framework.api.ConnectorFacade; -import org.identityconnectors.framework.common.objects.Schema; import org.testng.IObjectFactory; import org.testng.ITestContext; import org.testng.annotations.Factory; @@ -70,7 +69,7 @@ public Object[] createInstances(ITestContext context) { Injector injector = getInjector(context); List result = new ArrayList(); - IObjectFactory objectFactory = null; + IObjectFactory objectFactory = new ObjectFactoryImpl(); for (Class testClass: getContractTestClasses(context)) { Constructor constructor = null; @@ -111,14 +110,4 @@ public Injector getInjector(ITestContext context) { public DataProvider getDataProvider(ITestContext context) { return ConnectorHelper.createDataProvider(); } - - private static class ContractTestFactory { - - private ConnectorFacade connectorFacade = null; - - private Schema schema = null; - - private IObjectFactory objectFactory = new ObjectFactoryImpl(); - - } } diff --git a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/rpc/WebSocketConnectionGroup.java b/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/rpc/WebSocketConnectionGroup.java index d08f6addf..3c692afbe 100644 --- a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/rpc/WebSocketConnectionGroup.java +++ b/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/rpc/WebSocketConnectionGroup.java @@ -71,7 +71,7 @@ public class WebSocketConnectionGroup private Encryptor encryptor = null; - private RemoteOperationContext operationContext = null; + private volatile RemoteOperationContext operationContext = null; private final AtomicBoolean isRunning = new AtomicBoolean(Boolean.TRUE); private final Set principals = new TreeSet(String.CASE_INSENSITIVE_ORDER); diff --git a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/FrameworkUtil.java b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/FrameworkUtil.java index 03f42dd12..ace6fb229 100644 --- a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/FrameworkUtil.java +++ b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/FrameworkUtil.java @@ -20,6 +20,7 @@ * "Portions Copyrighted [year] [name of copyright owner]" * ==================== * Portions Copyrighted 2010-2016 ForgeRock AS. + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.common; @@ -485,14 +486,14 @@ public static Uid getUidIfGetOperation(Filter filter) { * * @return the framework version; never null. */ - public static Version getFrameworkVersion() { + public static synchronized Version getFrameworkVersion() { if (frameworkVersion == null) { frameworkVersion = Version.create(1, 5); } return frameworkVersion; } - static Version getFrameworkVersion(ClassLoader loader) throws IOException { + static Version readFrameworkVersion(ClassLoader loader) throws IOException { InputStream stream = loader.getResourceAsStream("connectors-framework.properties"); try { Properties props = new Properties(); diff --git a/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/framework/common/FrameworkUtilTests.java b/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/framework/common/FrameworkUtilTests.java index ff2186a62..d7052a7f3 100644 --- a/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/framework/common/FrameworkUtilTests.java +++ b/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/framework/common/FrameworkUtilTests.java @@ -19,6 +19,7 @@ * enclosed by brackets [] replaced by your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" * ==================== + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.common; @@ -41,13 +42,13 @@ public class FrameworkUtilTests { @Test public void testFrameworkVersion() throws Exception { ClassLoader loader = new VersionClassLoader(this.getClass().getClassLoader(), "1.2.3-alpha"); - assertEquals(FrameworkUtil.getFrameworkVersion(loader), Version.parse("1.2.3")); + assertEquals(FrameworkUtil.readFrameworkVersion(loader), Version.parse("1.2.3")); } @Test public void testFrameworkVersionCannotBeBlank() throws Exception { try { - FrameworkUtil.getFrameworkVersion(new VersionClassLoader(this.getClass().getClassLoader(), " ")); + FrameworkUtil.readFrameworkVersion(new VersionClassLoader(this.getClass().getClassLoader(), " ")); Assert.fail(); } catch (IllegalStateException e) { // OK. diff --git a/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/BatchRemoteCache.java b/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/BatchRemoteCache.java index 848ec7485..62b2f024b 100644 --- a/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/BatchRemoteCache.java +++ b/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/BatchRemoteCache.java @@ -20,6 +20,7 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.testconnector; @@ -61,7 +62,7 @@ public CachedBatchResult(Boolean complete, Boolean error, String resultId, Objec private final Map> tasks = new HashMap>(); private final Map> results = new HashMap>(); private final Map complete = new HashMap(); - private final String resultLock = "resultLock"; + private final Object resultLock = new Object(); public static void addTasks(String token, List tasklist) { singleton.tasks.put(token, new ArrayList(tasklist)); diff --git a/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstStatefulConnectorConfig.java b/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstStatefulConnectorConfig.java index d50ba0110..67b2459f5 100755 --- a/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstStatefulConnectorConfig.java +++ b/OpenICF-java-framework/testbundlev1/src/main/java/org/identityconnectors/testconnector/TstStatefulConnectorConfig.java @@ -123,7 +123,7 @@ public void setRandomString(String randomString) { private UUID guid; - private ScheduledExecutorService executorService = null; + private volatile ScheduledExecutorService executorService = null; public synchronized UUID getGuid() { if (null == guid) { From 5c4bcf277f8667aae45e73a1ed0336659d1d2cb5 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 2 Oct 2026 21:53:22 +0300 Subject: [PATCH 2/4] Publish the Groovy engine only after its customizer ran, and drop TestNG internals from ContractITCase ScriptedConfiguration.getGroovyScriptEngine() stored the engine before initializeCustomizer() ran: a failed customizer left an uncustomized engine in place for good, and a concurrent caller could get it mid-customization (#148). The fast path also read the volatile field twice while release() may null it in between (#149). The field is now read once, and the engine is published only after the customizer succeeds; the customizer's own re-entrant calls get it from a lock-guarded field. ContractITCase.createInstances instantiated org.testng.internal.ObjectFactoryImpl on every call; TestNG 7.5 moved that class and 7.10.2 removed IObjectFactory. It now calls the (String) constructor through plain reflection. Fixes #148 Fixes #149 --- .../scriptedcommon/ScriptedConfiguration.java | 24 +++- .../ScriptedConfigurationTest.java | 126 ++++++++++++++++++ .../contract/test/ContractITCase.java | 10 +- 3 files changed, 150 insertions(+), 10 deletions(-) create mode 100644 OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java diff --git a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java index f8aa4443f..237331a96 100644 --- a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java +++ b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java @@ -782,7 +782,14 @@ protected Log getLogger(final Class clazz) { return logger; } - private volatile GroovyScriptEngine groovyScriptEngine = null; + /** The engine, published only once its customizer has run. Guarded by {@code this}. */ + private GroovyScriptEngine groovyScriptEngine = null; + + /** + * The engine whose customizer is running, for the calls the customizer makes back into + * {@link #getGroovyScriptEngine()} on the same thread. Guarded by {@code this}. + */ + private GroovyScriptEngine customizingGroovyScriptEngine = null; /** * Synchronised for the whole initialisation, not double-checked: the @@ -792,6 +799,10 @@ protected Log getLogger(final Class clazz) { */ protected synchronized GroovyScriptEngine getGroovyScriptEngine() { if (null == groovyScriptEngine) { + if (null != customizingGroovyScriptEngine) { + return customizingGroovyScriptEngine; + } + final CompilerConfiguration compilerConfiguration = new CompilerConfiguration(config); compilerConfiguration.addCompilationCustomizers(getImportCustomizer(null)); @@ -799,10 +810,17 @@ protected synchronized GroovyScriptEngine getGroovyScriptEngine() { final GroovyClassLoader loader = new GroovyClassLoader(getParentLoader(), compilerConfiguration, true); - groovyScriptEngine = + final GroovyScriptEngine engine = new GroovyScriptEngine(getRoots(compilerConfiguration, loader), loader); - initializeCustomizer(); + // If the customizer fails, the engine is dropped and the next call retries. + customizingGroovyScriptEngine = engine; + try { + initializeCustomizer(); + } finally { + customizingGroovyScriptEngine = null; + } + groovyScriptEngine = engine; } return groovyScriptEngine; } diff --git a/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java new file mode 100644 index 000000000..4e975cc09 --- /dev/null +++ b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java @@ -0,0 +1,126 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.misc.scriptedcommon; + +import static org.fest.assertions.api.Assertions.assertThat; +import static org.fest.assertions.api.Assertions.fail; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; + +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import groovy.util.GroovyScriptEngine; + +/** + * Tests that {@link ScriptedConfiguration#getGroovyScriptEngine()} only hands out an engine whose + * customizer script has run. + */ +public class ScriptedConfigurationTest { + + /** What the customizer script does; set by each test. */ + private static volatile Runnable customizer; + + /** Called from the customizer script written by {@link #setUp()}. */ + public static void customize() { + customizer.run(); + } + + private File scriptRoot; + + private ScriptedConfiguration configuration; + + private ExecutorService executor; + + @BeforeMethod + public void setUp() throws Exception { + scriptRoot = Files.createTempDirectory("scripted-configuration").toFile(); + Files.write(new File(scriptRoot, "Customizer.groovy").toPath(), + (ScriptedConfigurationTest.class.getName() + ".customize()\n") + .getBytes(StandardCharsets.UTF_8)); + configuration = new ScriptedConfiguration(); + configuration.setScriptRoots(new String[] { scriptRoot.getAbsolutePath() }); + configuration.setCustomizerScriptFileName("Customizer.groovy"); + executor = Executors.newCachedThreadPool(); + } + + @AfterMethod + public void tearDown() throws Exception { + customizer = null; + executor.shutdownNow(); + new File(scriptRoot, "Customizer.groovy").delete(); + scriptRoot.delete(); + } + + @Test + public void testFailedCustomizerIsRetriedOnTheNextCall() { + final AtomicInteger calls = new AtomicInteger(); + customizer = () -> { + if (calls.incrementAndGet() == 1) { + throw new IllegalStateException("customizer failed"); + } + }; + + try { + configuration.getGroovyScriptEngine(); + fail("The customizer failure must reach the caller"); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("customizer failed"); + } + + assertThat(configuration.getGroovyScriptEngine()).isNotNull(); + assertThat(calls.get()).isEqualTo(2); + } + + @Test(timeOut = 30000) + public void testEngineIsHiddenFromOtherThreadsUntilCustomized() throws Exception { + final CountDownLatch customizing = new CountDownLatch(1); + final CountDownLatch finishCustomizing = new CountDownLatch(1); + customizer = () -> { + customizing.countDown(); + try { + finishCustomizing.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }; + + Future first = executor.submit(configuration::getGroovyScriptEngine); + customizing.await(); + Future second = executor.submit(configuration::getGroovyScriptEngine); + try { + GroovyScriptEngine early = second.get(500, TimeUnit.MILLISECONDS); + fail("Got " + early + " while the customizer was still running"); + } catch (TimeoutException expected) { + // the second caller waits for the customization to finish + } + + finishCustomizing.countDown(); + assertThat(first.get()).isNotNull(); + assertThat(second.get()).isSameAs(first.get()); + } +} diff --git a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java index 0002b15d6..b2431a346 100644 --- a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java +++ b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java @@ -25,7 +25,6 @@ package org.identityconnectors.contract.test; -import java.lang.reflect.Constructor; import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; @@ -33,10 +32,8 @@ import org.identityconnectors.common.StringUtil; import org.identityconnectors.contract.data.DataProvider; -import org.testng.IObjectFactory; import org.testng.ITestContext; import org.testng.annotations.Factory; -import org.testng.internal.ObjectFactoryImpl; import com.google.inject.Guice; import com.google.inject.Injector; @@ -69,17 +66,16 @@ public Object[] createInstances(ITestContext context) { Injector injector = getInjector(context); List result = new ArrayList(); - IObjectFactory objectFactory = new ObjectFactoryImpl(); for (Class testClass: getContractTestClasses(context)) { - Constructor constructor = null; try { - constructor = testClass.getConstructor(String.class); - Object test = objectFactory.newInstance(constructor, ""); + Object test = testClass.getConstructor(String.class).newInstance(""); injector.injectMembers(test); result.add(test); } catch (NoSuchMethodException e) { result.add(injector.getInstance(testClass)); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException("Cannot instantiate " + testClass.getName(), e); } } return result.toArray(); From 63e2ae950e90fbaae79a88b82d03bacc777d30d1 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sun, 4 Oct 2026 12:48:16 +0300 Subject: [PATCH 3/4] Unregister a failed customizer class, and wait for the blocked caller in the test Since a failed customizer is now retried on the next call, each failure compiled a new customizer class. The REST, CREST and SSH configurations register an ExpandoMetaClass on it, which keeps its GroovyClassLoader alive, so a customizer that keeps failing leaked one loader per call. initializeCustomizer() now removes the class with InvokerHelper.removeClass when the customizer fails. The comment on initializeCustomizer() states that the customizer runs under the configuration's monitor and must not wait for another thread that uses the engine. testEngineIsHiddenFromOtherThreadsUntilCustomized took a 500 ms timeout as proof that the second caller blocked; it now waits until that thread is BLOCKED or done, so an early publish fails the test on every run. --- .../scriptedcommon/ScriptedConfiguration.java | 12 +++- .../ScriptedConfigurationTest.java | 56 ++++++++++++++++--- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java index 237331a96..117fb7a63 100644 --- a/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java +++ b/OpenICF-groovy-connector/src/main/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfiguration.java @@ -827,11 +827,14 @@ protected synchronized GroovyScriptEngine getGroovyScriptEngine() { /* * This must be called once from thread-safe location and inside the - * synchronized to avoid deadlock. + * synchronized to avoid deadlock. The customizer runs while this + * configuration's monitor is held: it must not wait for another thread + * that calls getGroovyScriptEngine(), evaluate() or loadScript(). */ private void initializeCustomizer() { + Class customizerClass = null; try { - Class customizerClass = getCustomizerClass(); + customizerClass = getCustomizerClass(); if (null != customizerClass) { Binding binding = new Binding(); @@ -839,6 +842,11 @@ private void initializeCustomizer() { createCustomizerScript(customizerClass, binding).run(); } } catch (Throwable t) { + if (null != customizerClass) { + // The retry compiles a new class; unregister this one, together with any + // metaclass createCustomizerScript() put on it, so its loader can be collected. + InvokerHelper.removeClass(customizerClass); + } logger.error(t, "Failed to customize the connector"); throw ConnectorException.wrap(t); } diff --git a/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java index 4e975cc09..b4bdf1ac7 100644 --- a/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java +++ b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java @@ -26,14 +26,18 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import org.codehaus.groovy.reflection.ClassInfo; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import groovy.lang.Binding; +import groovy.lang.ExpandoMetaClass; +import groovy.lang.GroovySystem; +import groovy.lang.Script; import groovy.util.GroovyScriptEngine; /** @@ -96,6 +100,38 @@ public void testFailedCustomizerIsRetriedOnTheNextCall() { assertThat(calls.get()).isEqualTo(2); } + @Test + public void testFailedCustomizerClassIsUnregistered() { + customizer = () -> { + throw new IllegalStateException("customizer failed"); + }; + final AtomicReference customizerClass = new AtomicReference<>(); + // Registers a metaclass on the customizer class, as the REST, CREST and SSH + // configurations do; a registered metaclass keeps the class's loader alive. + configuration = new ScriptedConfiguration() { + @Override + protected Script createCustomizerScript(Class clazz, Binding binding) { + customizerClass.set(clazz); + ExpandoMetaClass metaClass = new ExpandoMetaClass(clazz, false, true); + metaClass.initialize(); + GroovySystem.getMetaClassRegistry().setMetaClass(clazz, metaClass); + return super.createCustomizerScript(clazz, binding); + } + }; + configuration.setScriptRoots(new String[] { scriptRoot.getAbsolutePath() }); + configuration.setCustomizerScriptFileName("Customizer.groovy"); + + try { + configuration.getGroovyScriptEngine(); + fail("The customizer failure must reach the caller"); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("customizer failed"); + } + + assertThat(customizerClass.get()).isNotNull(); + assertThat(ClassInfo.getClassInfo(customizerClass.get()).getStrongMetaClass()).isNull(); + } + @Test(timeOut = 30000) public void testEngineIsHiddenFromOtherThreadsUntilCustomized() throws Exception { final CountDownLatch customizing = new CountDownLatch(1); @@ -111,13 +147,17 @@ public void testEngineIsHiddenFromOtherThreadsUntilCustomized() throws Exception Future first = executor.submit(configuration::getGroovyScriptEngine); customizing.await(); - Future second = executor.submit(configuration::getGroovyScriptEngine); - try { - GroovyScriptEngine early = second.get(500, TimeUnit.MILLISECONDS); - fail("Got " + early + " while the customizer was still running"); - } catch (TimeoutException expected) { - // the second caller waits for the customization to finish + final AtomicReference secondThread = new AtomicReference<>(); + Future second = executor.submit(() -> { + secondThread.set(Thread.currentThread()); + return configuration.getGroovyScriptEngine(); + }); + // The second caller either returns early or blocks on the configuration's monitor. + while (!second.isDone() && (secondThread.get() == null + || secondThread.get().getState() != Thread.State.BLOCKED)) { + Thread.sleep(10); } + assertThat(second.isDone()).as("second caller returned while the customizer ran").isFalse(); finishCustomizing.countDown(); assertThat(first.get()).isNotNull(); From 2e6e8d7ecd3f48ddc5d76f6f865b262f59276fbd Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 5 Oct 2026 16:18:53 +0300 Subject: [PATCH 4/4] Reset the release closure in the SSH and CREST customize DSL A failed customizer is retried on the next call (#148). When the failed attempt registered release {} and the successful retry did not, release() still ran the failed attempt's closure: SSH's customize never reset it, and CREST's "release = null" wrote a private field that nothing reads. Both now clear it with setReleaseClosure(null), the way REST's customize already resets its closure, and CREST's unread field is gone. --- .../ScriptedCRESTConfiguration.groovy | 5 +- .../ScriptedConfigurationTest.java | 47 +++++++++ .../connectors/ssh/SSHConfiguration.groovy | 2 + .../SSHConfigurationCustomizerTest.java | 99 +++++++++++++++++++ 4 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 OpenICF-ssh-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/SSHConfigurationCustomizerTest.java diff --git a/OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedcrest/ScriptedCRESTConfiguration.groovy b/OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedcrest/ScriptedCRESTConfiguration.groovy index 921f9c849..fd0fd5b02 100644 --- a/OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedcrest/ScriptedCRESTConfiguration.groovy +++ b/OpenICF-groovy-connector/src/main/groovy/org/forgerock/openicf/connectors/scriptedcrest/ScriptedCRESTConfiguration.groovy @@ -20,6 +20,8 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" + * + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.openicf.connectors.scriptedcrest @@ -198,7 +200,6 @@ class ScriptedCRESTConfiguration extends ScriptedConfiguration { } private Closure init = null; - private Closure release = null; private Closure beforeRequest = null; private Closure onComplete = null; private Closure onFail = null; @@ -223,7 +224,7 @@ class ScriptedCRESTConfiguration extends ScriptedConfiguration { customizerClass.metaClass.customize << { Closure cl -> init = null - release = null + setReleaseClosure(null) beforeRequest = null onComplete = null onFail = null diff --git a/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java index b4bdf1ac7..404918062 100644 --- a/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java +++ b/OpenICF-groovy-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/ScriptedConfigurationTest.java @@ -30,6 +30,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.codehaus.groovy.reflection.ClassInfo; +import org.forgerock.openicf.connectors.scriptedcrest.ScriptedCRESTConfiguration; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -54,6 +55,22 @@ public static void customize() { customizer.run(); } + /** How often {@link #isFirstAttempt()} was called; reset by {@link #setUp()}. */ + private static final AtomicInteger attempts = new AtomicInteger(); + + /** Whether the release closure of a customizer script ran; reset by {@link #setUp()}. */ + private static volatile boolean releaseClosureRan; + + /** Called from a customizer script to behave differently on its first attempt. */ + public static boolean isFirstAttempt() { + return attempts.incrementAndGet() == 1; + } + + /** Called from the release closure of a customizer script. */ + public static void releaseClosureRan() { + releaseClosureRan = true; + } + private File scriptRoot; private ScriptedConfiguration configuration; @@ -62,6 +79,8 @@ public static void customize() { @BeforeMethod public void setUp() throws Exception { + attempts.set(0); + releaseClosureRan = false; scriptRoot = Files.createTempDirectory("scripted-configuration").toFile(); Files.write(new File(scriptRoot, "Customizer.groovy").toPath(), (ScriptedConfigurationTest.class.getName() + ".customize()\n") @@ -132,6 +151,34 @@ protected Script createCustomizerScript(Class clazz, Binding binding) { assertThat(ClassInfo.getClassInfo(customizerClass.get()).getStrongMetaClass()).isNull(); } + @Test + public void testRetriedCRESTCustomizerDropsTheReleaseClosureOfTheFailedAttempt() throws Exception { + final String test = ScriptedConfigurationTest.class.getName(); + Files.write(new File(scriptRoot, "Customizer.groovy").toPath(), + ("if (" + test + ".isFirstAttempt()) {\n" + + " customize {\n" + + " release { " + test + ".releaseClosureRan() }\n" + + " }\n" + + " throw new IllegalStateException('customizer failed')\n" + + "}\n" + + "customize {\n" + + "}\n").getBytes(StandardCharsets.UTF_8)); + configuration = new ScriptedCRESTConfiguration(); + configuration.setScriptRoots(new String[] { scriptRoot.getAbsolutePath() }); + configuration.setCustomizerScriptFileName("Customizer.groovy"); + + try { + configuration.getGroovyScriptEngine(); + fail("The customizer failure must reach the caller"); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("customizer failed"); + } + assertThat(configuration.getGroovyScriptEngine()).isNotNull(); + + configuration.release(); + assertThat(releaseClosureRan).as("release closure of the failed attempt ran").isFalse(); + } + @Test(timeOut = 30000) public void testEngineIsHiddenFromOtherThreadsUntilCustomized() throws Exception { final CountDownLatch customizing = new CountDownLatch(1); diff --git a/OpenICF-ssh-connector/src/main/groovy/org/forgerock/openicf/connectors/ssh/SSHConfiguration.groovy b/OpenICF-ssh-connector/src/main/groovy/org/forgerock/openicf/connectors/ssh/SSHConfiguration.groovy index adc913b6c..39ace1e63 100644 --- a/OpenICF-ssh-connector/src/main/groovy/org/forgerock/openicf/connectors/ssh/SSHConfiguration.groovy +++ b/OpenICF-ssh-connector/src/main/groovy/org/forgerock/openicf/connectors/ssh/SSHConfiguration.groovy @@ -12,6 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.forgerock.openicf.connectors.ssh @@ -388,6 +389,7 @@ public class SSHConfiguration extends ScriptedConfiguration { init = null onCreateConnection = null onCloseConnection = null + setReleaseClosure(null) def delegate = [ init : { Closure paramClosure -> diff --git a/OpenICF-ssh-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/SSHConfigurationCustomizerTest.java b/OpenICF-ssh-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/SSHConfigurationCustomizerTest.java new file mode 100644 index 000000000..a130914f4 --- /dev/null +++ b/OpenICF-ssh-connector/src/test/java/org/forgerock/openicf/misc/scriptedcommon/SSHConfigurationCustomizerTest.java @@ -0,0 +1,99 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.misc.scriptedcommon; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.fail; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.concurrent.atomic.AtomicInteger; + +import org.forgerock.openicf.connectors.ssh.SSHConfiguration; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Tests the {@code customize} DSL of {@link SSHConfiguration} when its customizer is retried. It + * lives in this package to reach {@link ScriptedConfiguration#getGroovyScriptEngine()}: a subclass + * would break the customizer's closures, which set private fields of {@link SSHConfiguration}. + */ +public class SSHConfigurationCustomizerTest { + + /** How often {@link #isFirstAttempt()} was called; reset by {@link #setUp()}. */ + private static final AtomicInteger attempts = new AtomicInteger(); + + /** Whether the release closure of the customizer script ran; reset by {@link #setUp()}. */ + private static volatile boolean releaseClosureRan; + + /** Called from the customizer script to behave differently on its first attempt. */ + public static boolean isFirstAttempt() { + return attempts.incrementAndGet() == 1; + } + + /** Called from the release closure of the customizer script. */ + public static void releaseClosureRan() { + releaseClosureRan = true; + } + + private File scriptRoot; + + @BeforeMethod + public void setUp() throws Exception { + attempts.set(0); + releaseClosureRan = false; + scriptRoot = Files.createTempDirectory("ssh-configuration").toFile(); + } + + @AfterMethod + public void tearDown() { + new File(scriptRoot, "Customizer.groovy").delete(); + scriptRoot.delete(); + } + + @Test + public void testRetriedCustomizerDropsTheReleaseClosureOfTheFailedAttempt() throws Exception { + final String test = SSHConfigurationCustomizerTest.class.getName(); + Files.write(new File(scriptRoot, "Customizer.groovy").toPath(), + ("if (" + test + ".isFirstAttempt()) {\n" + + " customize {\n" + + " release { " + test + ".releaseClosureRan() }\n" + + " }\n" + + " throw new IllegalStateException('customizer failed')\n" + + "}\n" + + "customize {\n" + + "}\n").getBytes(StandardCharsets.UTF_8)); + final SSHConfiguration configuration = new SSHConfiguration(); + configuration.setScriptRoots(new String[] { scriptRoot.getAbsolutePath() }); + configuration.setCustomizerScriptFileName("Customizer.groovy"); + + try { + configuration.getGroovyScriptEngine(); + fail("The customizer failure must reach the caller"); + } catch (IllegalStateException e) { + assertEquals(e.getMessage(), "customizer failed"); + } + assertNotNull(configuration.getGroovyScriptEngine()); + + configuration.release(); + assertFalse(releaseClosureRan, "release closure of the failed attempt ran"); + } +}