Conversation
…nterned-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.
…tNG 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 (OpenIdentityPlatform#148). The fast path also read the volatile field twice while release() may null it in between (OpenIdentityPlatform#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 OpenIdentityPlatform#148 Fixes OpenIdentityPlatform#149
… 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.
A failed customizer is retried on the next call (OpenIdentityPlatform#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.
release() dropped the engine but left the class of its customizer registered. The REST, CREST and SSH configurations put an ExpandoMetaClass on that class, which on Groovy 2.4.21 keeps the class, its GroovyClassLoader with every compiled script, and the released configuration alive: one loader leaked per facade dispose or configuration change. getGroovyScriptEngine() now keeps the customizer class of the published engine, and release() removes it with InvokerHelper.removeClass after the release closure has run. Fixes OpenIdentityPlatform#154
vharseko
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #154
ScriptedConfiguration.release()dropped the published engine but left the class of its customizer script registered. The REST, CREST and SSH configurations register anExpandoMetaClasson that class increateCustomizerScript(customizerClass.metaClass.customize << {…}). On Groovy 2.4.21 a registered metaclass keeps the class reachable, and with it the configuration's ownGroovyClassLoader, every script it compiled, and the released configuration that owns thecustomizeclosure. So every facade dispose or configuration change of a REST, CREST or SSH connector leaked one loader. #132 covers only a customizer that fails.ScriptedConfiguration.initializeCustomizer()nullwhen there is none)ScriptedConfiguration.getGroovyScriptEngine()publishedCustomizerClass, guarded by the configuration's monitor and set together with the published engineScriptedConfiguration.release()InvokerHelper.removeClass(publishedCustomizerClass)and clears the field. The REST, CREST and SQL overrides ofrelease()all callsuper.release()Test.
ScriptedConfigurationTest.testReleaseUnregistersThePublishedCustomizerClassuses the stand-in fromtestFailedCustomizerClassIsUnregistered(now the shared helperregisteringMetaClassOnCustomizer) with a customizer that succeeds. It checks that the metaclass is registered aftergetGroovyScriptEngine(), then callsrelease()and checks thatClassInfo.getClassInfo(clazz).getStrongMetaClass()isnull. Without therelease()change it fails on that last assertion.Local run:
mvn -pl OpenICF-groovy-connector,OpenICF-ssh-connector -am test. groovy-connector: 131 run, 0 failed. The skipped tests are the samples that need external systems (SQL/REST/CREST/Azure/ARM, Linux/Kerberos).SSHConfigurationCustomizerTestpassed.