Add initializer for in-process Gradle executor #29142
Conversation
Avoiding strange static state caused by unit tests that causes flaky test failures.
| void start() { | ||
| if (GradleContextualExecuter.embedded ) { | ||
| AbstractGradleExecuter.GLOBAL_SERVICES.get(ObjectFactory) | ||
| } |
There was a problem hiding this comment.
Can we add some more comments about why this is happening only in embedded mode, and why this specific service is being queried?
There was a problem hiding this comment.
Is the Javadoc on the class enough for describing why it only happens for embedded mode? I can add something here why we query ObjectFactory.
There was a problem hiding this comment.
I left some more comments.
| @@ -115,6 +116,7 @@ public abstract class AbstractGradleExecuter implements GradleExecuter, Resettab | |||
| .build(); | |||
|
|
|||
| private static final JvmVersionDetector JVM_VERSION_DETECTOR = GLOBAL_SERVICES.get(JvmVersionDetector.class); | |||
| private static final ObjectFactory objectFactory = GLOBAL_SERVICES.get(ObjectFactory.class); | |||
There was a problem hiding this comment.
Isn't this an unused field now? Can we clarify with a comment?
There was a problem hiding this comment.
This was a leftover, I removed it.
This was a leftover from some tryout
| * may have a test implementation instead of the real thing when a unit test runs first. | ||
| */ | ||
| @Issue("https://github.com/gradle/gradle-private/issues/3534") | ||
| public class InProcessGradleExecutorInitialization implements IGlobalExtension { |
There was a problem hiding this comment.
💅 maybe reference this class in the InProcessGradleExecutor javadocs too? Global extensions are not something used often in our codebase (in fact, I've learned about them right now), and the extra visibility can save some time in the future for people figuring out how the executer is being initialized.
|
Shall we merge this? |
Avoiding strange static state caused by unit tests that causes flaky test failures.
The real culprit seems to be the reuse of the cache inside the
AsmBackedClassGenerator:gradle/subprojects/model-core/src/main/java/org/gradle/internal/instantiation/generator/AsmBackedClassGenerator.java
Lines 173 to 179 in d700905
If some test (e.g.
WorkerProcessIntegrationTest) creates anAsmBackedClassGeneratorwith theTestCrossBuildInMemoryCacheFactoryfirst, then all other tests will get aTestCacheas the cache implementation. TheTestCacheis not thread-safe, and theConcurrentClassDecorationSpecwould break.Making
TestCachethread-safe fixes the immediate breakage of theConcurrentClassDecorationSpec, but this is only a symptom and doesn't address the root cause. So we add the initializer for Global services instead so we don't get into the situation.