Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect test candidates eagerly #18

Merged
merged 1 commit into from Aug 14, 2022
Merged

Collect test candidates eagerly #18

merged 1 commit into from Aug 14, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 4, 2022

concat produces a lazy sequence, which makes its repeated usage in a
loop a potential stack overflow hazard (when the produced seq gets
realized and has to recur over its subseqs).

Background: I've been trying to introduce clojure to a java/kotlin project. Clojure tests kept on dying with a StackOverflowError in CI during test discovery, with stacktrace mentioning concat. I couldn't debug the issue very well - it wasn't reliably reproducible locally - but noticed that apparently all AOTted classes in test classpath were being scanned for tests, which is a lot of classes. I've run grep on jovial, found a single use of concat in a loop and tried to replace it with a non-lazy equivalent. Tests run fine with the patched version. I'm not 100% convinced that concat's laziness was the sole culprit due to flakiness of local test runs, but if it helps it helps.

`concat` produces a lazy sequence, which makes its repeated usage in a
loop a potential stack overflow hazard (when the produced seq gets
realized and has to recur over its subseqs).
@ghost
Copy link
Author

ghost commented Mar 7, 2022

I've actually taken the time to sit down and repro the overflow, it's proven to not be too hard. It's here, but I suspect tweaking the numbers in infinite-fns and/or creating more namespaces (and importing them in the test) might be necessary depending on the local JVM, machine and alignment of stars.

The stacktrace in JUnit report (truncated, of course):

org.gradle.api.internal.tasks.testing.TestSuiteExecutionException: Could not complete execution for Gradle Test Executor 1.
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
	at java.base@17.0.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base@17.0.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base@17.0.1/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base@17.0.1/java.lang.reflect.Method.invoke(Method.java:568)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at jdk.proxy1/jdk.proxy1.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: org.junit.platform.commons.JUnitException: TestEngine with ID 'org.ajoberstar.jovial.clojure-test' failed to discover tests
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:111)
	at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:85)
	at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:92)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	... 18 more
Caused by: java.lang.StackOverflowError
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:51)
	at clojure.lang.RT.seq(RT.java:535)
	at clojure.core$seq__5419.invokeStatic(core.clj:139)
	at clojure.core$concat$fn__5510.invoke(core.clj:727)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:51)
	at clojure.lang.RT.seq(RT.java:535)
	at clojure.core$seq__5419.invokeStatic(core.clj:139)
	at clojure.core$concat$fn__5510.invoke(core.clj:727)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:51)
	at clojure.lang.RT.seq(RT.java:535)

@ajoberstar ajoberstar merged commit 32a98bf into clojurephant:main Aug 14, 2022
@ajoberstar
Copy link
Member

Apologies for the delay on this, I lost track of this PR being open. I appreciate the work you did to dig into this and fix this! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

1 participant