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

Add the contract to runBlocking for shared JVM/Native code #4368

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Collaborator

Additionally, on Native, make thread keepalive checks a bit more efficient.

@dkhalanskyjb dkhalanskyjb marked this pull request as draft February 26, 2025 14:58
@dkhalanskyjb
Copy link
Collaborator Author

7 tests failed on the CI. Usually, that's pretty conclusive, but I fail to reproduce any of the failures on my machine. What's especially mysterious is that the tests failed on the JVM, where I don't think I did anything but 1) refactor the code somewhat and 2) remove the @Throws(InterruptedException) annotation.

@dkhalanskyjb
Copy link
Collaborator Author

I couldn't reproduce the issue locally after running a clean build in a loop for six hours (after which the Gradle daemon failed with an OOM, which is fun, but probably unrelated). Restarting the build on the CI, I don't observe the failure for the second time. Still, please review this especially carefully, because something did go wrong, and I have no idea what did.

The failures:

  • A couple of println calls in the most mysterious place:
  at java.base/java.io.PrintStream.println(PrintStream.java:882)
  at org.openjdk.jol.vm.VM.current(VM.java:73)
  at org.openjdk.jol.layouters.CurrentLayouter.layout(CurrentLayouter.java:52)
  at org.openjdk.jol.info.ClassLayout.parseClass(ClassLayout.java:72)
  at org.openjdk.jol.info.ClassLayout.parseClass(ClassLayout.java:56)
  at kotlinx.coroutines.MemoryFootprintTest.assertLayout(MemoryFootprintTest.kt:34)
  • A couple of println calls in TestBase.println. So, previousOut was set incorrectly, which is indeed possible if a test has finished without calling finish! Here's a fix for that and some other build failures: Improve test robustness #4369. It also makes it easier to understand what the mysterious println call was trying to tell us.
  • A couple of threads had the wrong name?

@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review February 27, 2025 12:11
Additionally, on Native, make thread keepalive checks a bit more
efficient.
@dkhalanskyjb dkhalanskyjb force-pushed the dk-runBlocking-native-contract branch from c038e59 to 088f61f Compare February 27, 2025 18:02
@dkhalanskyjb dkhalanskyjb removed the request for review from qwwdfsad February 28, 2025 06:17
@dkhalanskyjb dkhalanskyjb marked this pull request as draft February 28, 2025 06:17
@dkhalanskyjb
Copy link
Collaborator Author

runBlocking's @Throws on the JVM is important, as without it, some Java code breaks: https://grep.app/search?f.lang=Java&q=BuildersKt.runBlocking

@dkhalanskyjb dkhalanskyjb force-pushed the dk-runBlocking-native-contract branch from 0970c29 to 57254ab Compare February 28, 2025 10:46
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review February 28, 2025 10:47
@dkhalanskyjb dkhalanskyjb force-pushed the dk-runBlocking-native-contract branch from 57254ab to 2d9d4ac Compare February 28, 2025 13:56
@qwwdfsad
Copy link
Collaborator

A couple of println calls in the most mysterious place

Depending on the Java version, JOL from MemoryFootprintTest sometimes prints warnings to stderr/stdout. Maybe it's worth @Ignoreing it and using only as part of the development, as it requires non-trivial command-line args setup with modern Java

});
} catch (InterruptedException e) {
}
assert entered;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using JUnit's assertFoo functions. Java's asserts can be disabled (or, to be more precise, can be not enabled). Especially for our custom Gradle setup, the simpler the thing -- the better

@@ -30,6 +30,11 @@ public final class kotlinx/coroutines/BuildersKt {
public static final fun withContext (Lkotlin/coroutines/CoroutineContext;Lkotlin/jvm/functions/Function2;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
}

public final class kotlinx/coroutines/Builders_concurrentKt {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that it's possible to workaround the need for that middle layer by adding

@file:JvmMultifileClass
@file:JvmName("BuildersKt")

to Builders.concurrent.kt. Could you please consider doing so?

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

Successfully merging this pull request may close these issues.

2 participants