Skip to content

Attempt to fix a flaky coroutine-dump-verifying test #4589

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.

Sign up for GitHub

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

Open
dkhalanskyjb wants to merge 2 commits into
base: develop
Choose a base branch
from
+20 −14
Open

Attempt to fix a flaky coroutine-dump-verifying test #4589

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fabf105
Attempt to fix a flaky coroutine-dump-verifying test
dkhalanskyjb Dec 11, 2025
3c44b9f
Clarify the yield() calls
dkhalanskyjb Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions kotlinx-coroutines-debug/test/CoroutinesDumpTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,27 @@ package kotlinx.coroutines.debug

import kotlinx.coroutines.testing.*
import kotlinx.coroutines.*
import org.junit.*
import org.junit.Test
import kotlin.coroutines.*
import kotlin.test.*

class CoroutinesDumpTest : DebugTestBase() {
private val monitor = Any()
private var coroutineThread: Thread? = null // guarded by monitor

@Before
@BeforeTest
override fun setUp() {
super.setUp()
DebugProbes.enableCreationStackTraces = true
}

@Test
fun testSuspendedCoroutine() = runBlocking {
val deferred = async(Dispatchers.Default) {
sleepingOuterMethod()
val latch = CompletableDeferred<Unit>()
val singleThreadedDispatcher = newSingleThreadContext("TestCoroutineThread")
val deferred = async(singleThreadedDispatcher) {
sleepingOuterMethod(singleThreadedDispatcher, latch)
}

awaitCoroutine()
latch.await()
val found = DebugProbes.dumpCoroutinesInfo().single { it.job === deferred }
verifyDump(
"Coroutine \"coroutine#1\":DeferredCoroutine{Active}@1e4a7dd4, state: SUSPENDED\n" +
Expand All @@ -35,6 +34,7 @@ class CoroutinesDumpTest : DebugTestBase() {
"\tat kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt)\n",
ignoredCoroutine = "BlockingCoroutine"
) {
singleThreadedDispatcher.close()
deferred.cancel()
coroutineThread!!.interrupt()
}
Expand Down Expand Up @@ -181,23 +181,29 @@ class CoroutinesDumpTest : DebugTestBase() {
private suspend fun nestedActiveMethod(shouldSuspend: Boolean) {
if (shouldSuspend) yield()
notifyCoroutineStarted()
while (coroutineContext[Job]!!.isActive) {
while (currentCoroutineContext()[Job]!!.isActive) {
try {
Thread.sleep(60_000)
} catch (_ : InterruptedException) {
}
}
}

private suspend fun sleepingOuterMethod() {
sleepingNestedMethod()
yield() // TCE
private suspend fun sleepingOuterMethod(currentDispatcher: CoroutineDispatcher, latch: CompletableDeferred<Unit>) {
sleepingNestedMethod(currentDispatcher, latch)
yield() // TCE: make sure `sleepingOuterMethod` is contained in the continuation of `sleepingNestedMethod`
Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

So this one avoids being inlined into the test method, so that sleepingOuterMethod definitely appears in the stackstrace

}

private suspend fun sleepingNestedMethod() {
yield() // Suspension point
notifyCoroutineStarted()
private suspend fun sleepingNestedMethod(currentDispatcher: CoroutineDispatcher, latch: CompletableDeferred<Unit>) {
/* Schedule a computation on the current single-threaded dispatcher.
Since that thread is currently running this code,
the start notification will happen *after* the currently running coroutine suspends. */
currentDispatcher.dispatch(currentDispatcher) {
coroutineThread = Thread.currentThread()
latch.complete(Unit)
}
delay(Long.MAX_VALUE)
yield() // TCE: make sure `sleepingNestedMethod` is contained in the continuation of `delay`
Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

And this one avoids being inlined into sleepingOuterMethod, so that sleepingNestedMethod definitely appears in the stacktrace

Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

I'm struggling to make the step from your comment to my understanding. Firstly, because I don't understand how's yield preventing inlining, and also how's the fact that yield is contained in a continuation prevents inlining.

Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

Oh wait, let me read your huge comment above, which I didn't see.

Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

I kind of see how it works now, but your comments skips a few steps of reasoning, so it's a bit hard to decipher without extra thinking or additional context.

Could add info to your comment:

  • Used as an explicit suspension point which enforces state-machine being generated for sleepingNestedMethod
  • Without the state machine, the delay's continuation is sleepingOuterMethod.

Copy link
Contributor

@murfel murfel Dec 15, 2025

Choose a reason for hiding this comment

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

And I don't think that "TCE" comment is useful, really, either here or above. It is in fact tail-call elimination elimination, TCEE.

Copy link
Collaborator Author

@dkhalanskyjb dkhalanskyjb Dec 15, 2025

Choose a reason for hiding this comment

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

There's a balance to strike so that the useful information contained in a comment doesn't get obscured by restating the knowledge that's implicitly required. Using yield() to ensure a suspend function's stack frame is preserved is a common pattern in our tests, and restating this everywhere feels excessive to me. In my opinion, https://github.com/Kotlin/KEEP/blob/main/proposals/KEEP-0164-coroutines.md, when taken into account together with the TCE mark, provide enough context for the yield().

In any case, this discussion doesn't feel relevant to the PR we're discussing. If you would like us to improve the documentation of the places where we disable the tail call optimization of suspend functions, we can do so in a separate change applied consistently in the codebase.

}

private fun awaitCoroutine() = synchronized(monitor) {
Expand Down