`TestScope.runTest` may cause thread starvation (JVM, Native)

[OliverO2]

The documentation for TestScope states.

If [context] provides a [Job], that job is used as a parent for the new scope.

This implies that TestScope is meant to be used inside an existing coroutine. In that case, runBlocking must be avoided to guard against possible thread starvation (c.f. #3983).

What happens now:

TestScope.runTest() currently calls createTestResult(), which translates into runBlocking on JVM and Native.

What should happen instead:

Only the top-level runTest functions should be allowed to call runBlocking. TestScope.runTest() should avoid calls to runBlocking.

Use case:

The TestBalloon test framework organizes test suites in a multi-level hierarchy, with a coroutine hierarchy mirroring the suite hierarchy. It uses TestScope.runTest to run individual tests at the lowest levels of that hierarchy, with virtual time and exception handling. TestBalloon can run tests in parallel using Dispatchers.Default. (Note: TestBalloon has its own mechanism to provide the necessary Promise to the JS-based infrastructure independent of TestScope.runTest().)

Users of TestBalloon have experienced thread starvation with parallel test invocation in conjunction with TestScope. Disabling TestScope did not show thread starvation. If a reproducer is required, we could work to provide one.

[dkhalanskyjb]

Hi!

This implies that TestScope is meant to be used inside an existing coroutine.

Nope, this entire premise is flawed. runTest is intended to be the root coroutine running in a test. We should update the documentation to clearly state that. Right now, it only says that it's similar in behavior to runBlocking, which may be too implicit.

Job instances do not necessarily correspond to coroutines. You could create a Job() or a CompletableDeferred() manually and pass that as an argument. A plausible reason for why you'd want to do that doesn't come to mind, and truth be told, we only added that behavior for uniformity between TestScope() and CoroutineScope(), without a specific intended use case. Another option would be to throw an exception if a Job was passed, but we didn't think of a way someone could have misused that functionality accidentally, so we left that in. Thank you for providing an example of a problematic behavior!

A reproducer for the problematic behavior is not necessary. Calling runBlocking inside Dispatchers.Default is known to be incorrect.

Only the top-level runTest functions should be allowed to call runBlocking. TestScope.runTest() should avoid calls to runBlocking.

The way you are describing this, it's not possible to implement without making runTest a suspend function. Since runTest has to wait for asynchronous computations in the spawned coroutines to finish—including cases when another thread is used—it needs to either block (hence runBlocking) or suspend while an indefinite amount of time passes.

We can consider making a separate suspend fun testScope(block: TestScope.() -> Unit), similar to coroutineScope or supervisorScope, that would run its block the way runTest runs a test, but runTest behaves as intended. Would you be interested in testScope?

[OliverO2]

Job instances do not necessarily correspond to coroutines.

The 1:1 correspondence was always part of my mental model. The distinction seemed to be an implementation detail.

Another option would be to throw an exception if a Job was passed

I'd view this as preferable it there is no constructive use case. In contrast to documentation, it would prevent users from accidentally launching a footgun that's difficult to debug.

We can consider making a separate suspend fun testScope(block: TestScope.() -> Unit), similar to coroutineScope or supervisorScope, that would run its block the way runTest runs a test, but runTest behaves as intended. Would you be interested in testScope?

That's an excellent idea and I'd much appreciate it.

Use case: With larger numbers of sufficiently isolated tests, running them in parallel on Dispatchers.Default has been demonstrated to show very high CPU utilization and greatly reduced overall runtimes for tests, like down from 4:53 to 1:19 minutes for one test series. This deep concurrency made kotlinx.coroutines shine when compared to conventional coarse-grained parallelism on upper invocation levels. And the test dispatcher's virtual time provides an ideal complement for parallel execution as real time makes less sense on a highly loaded system.

So having a separate suspend fun testScope(block: TestScope.() -> Unit) would support this use case with less coupling due to a smaller API surface, compared to re-implementing virtual time outside kotlinx.coroutines.

Am I assuming correctly that if I'd provide a TestDispatcher or its scheduler to that testScope function as part of its coroutineContext, we must make sure that such TestDispatcher (or its scheduler) cannot be shared across multiple testScope functions running in parallel?

[dkhalanskyjb]

The distinction seemed to be an implementation detail.

The simplest way to observe the distinction is to manually create a coroutine scope using the CoroutineScope() constructor function. It's not a coroutine, but it has a Job, which can be used to cancel the whole collection of coroutines launched in that scope. So, Job is a general mechanism for constructing lifetime hierarchies in kotlinx.coroutines.

Am I assuming correctly that if I'd provide a TestDispatcher or its scheduler to that testScope function

That is a difficult question.

In the ideal world, the entirety of the test forms a single structured concurrency hierarchy, with TestScope at the top and every child Job descending from it. However, people pass test dispatchers to the systems under test without making a child scope to inject: https://grep.app/search?f.lang=Kotlin&q=%3A+CoroutineDispatcher. Then, the only indicator that some work created by the test hasn't completed is a task in a test dispatcher.

(While writing this, I realized that our test framework is dealing with that situation worse than it could be: https://github.com/Kotlin/kotlinx.coroutines/issues/4580)

If a test scheduler can be shared between tests, then tests can no longer rely on the dispatcher being empty. This also can't be solved by doing testScope { body() }; testScheduler.advanceUntilIdle(), as test scheduler tasks may want to create new child coroutines for TestScope. It would be very strange to prohibit them from doing so, as they are still part of the test, the test hasn't logically completed by that time.

To sum it up, the only way we can support dispatcher injection in systems under test without creating child scopes of TestScope is to make each test responsible for its own coroutine scheduler.

That said, testScope may be explicitly implemented as a building block for third-party test frameworks, in which case, other behaviors are possible if the test framework is stricter about structured concurrency. Do you think it would be viable to prohibit TestBalloon users from injecting dispatchers into coroutines that are not children of the TestScope?

[OliverO2]

So, Job is a general mechanism for constructing lifetime hierarchies in kotlinx.coroutines.

Understood. Could that be simplified by dropping the distinction between Job and coroutine, and make users create a coroutine per lifetime hierarchy level instead?

Do you think it would be viable to prohibit TestBalloon users from injecting dispatchers into coroutines that are not children of the TestScope?

Yes, if I change the execution model: Currently, a TestScope is created per test. Tests form the lowest level of the test element hierarchy, which mirrors a coroutine hierarchy. Tests can request access to shared state via fixtures. Such fixtures can be created at any (test suite) level in the test element hierarchy, have a lifetime of that level's execution, and execute within that level's coroutine.

Now what I could do is drop the ability to create a TestSope per test, and restrict it to the compartment level. Test compartments form the second level of the execution hierarchy, and always run in isolation. So I could guarantee that all tests (and the fixtures they use) either run in the same TestScope or they don't have a TestScope at all and won't get one (unless they'd explicitly call kotlinx.coroutines functions to do so).

Would that make sense?

Could I allow parallelism under such a TestScope and still share virtual time across all coroutines executing in that TestScope?

[dkhalanskyjb]

Could I allow parallelism under such a TestScope and still share virtual time across all coroutines executing in that TestScope?

Then, if a test launches some computations outside structured concurrency, the test will have no way of knowing some computations are still pending, which will lead to test cross-contamination. I.e., one test launches something, and some other test fails because it observes a failure. This greatly confuses the users, which we observed in a mild form because of uncaught exception handlers being installed globally: https://github.com/Kotlin/kotlinx.coroutines/issues?q=uncaughtexceptionsbeforetest

[OliverO2]

Then, if a test launches some computations outside structured concurrency

Isn't that almost always a recipe for a planned disaster which we cannot avoid if someone insists?

But I'd expect that to be way less pronounced with TestBalloon. Its scope-friendly architecture encourages users (by making it easy) to stay within structured concurrency at all times:

• If users need to run services, they can easily do so with fixtures, which automatically close and their coroutines get cancelled at the end of their lifetime.
• If users need to use a Main dispatcher, they'd put them into a safe compartment for it, or use TestConfig.mainDispatcher().
• Replacing the classic static test setup (including @Before and @After) with a properly scoped structure avoids typical footguns when dealing with structured concurrency and other scope-oriented designs.

Having said that, there is a responsibility to design a test setup in a reasonable way. That responsibility ultimately rests with the users. Even more so if tests run concurrently. We can make things as simple as possible, but not simpler.

So it sounds like:

1. The proposed suspend fun testScope(block: TestScope.() -> Unit) would be a viable way to safely provide a TestScope to a number of tests running concurrently on Dispatchers.Default.
2. If the TestDispatcher could still handle virtual time in the presence of parallel execution, that would be ideal. (If not, I could restrict that to single-threaded execution.)

If that's OK with you, I'd much appreciate such a testScope() implementation coming to kotlinx.coroutines and would be happy to integrate it into TestBalloon.

[dkhalanskyjb]

The proposed suspend fun testScope(block: TestScope.() -> Unit) would be a viable way to safely provide a TestScope to a number of tests running concurrently on Dispatchers.Default.

I'd like to propose a different hierarchy: each test receives a testScope, whose logic runs on Dispatchers.Default. The test body runs on its own delay-skipping dispatcher.

If the primary goal of this proposal is to run tests in parallel, then I don't understand the need to share test dispatchers among different tests. Each test could get its own, independent test dispatcher, test coroutine scheduler, and TestScope could it not? Then, the same guarantees as runTest could still be provided, only in a suspend form.

Isn't that almost always a recipe for a planned disaster which we cannot avoid if someone insists?

You could say that, but kotlinx.coroutines hasn't historically imposed this view on the users and attempted to remain flexible in the face of different usage scenarios. Most notably, a typical Android codebase most likely uses a viewModelScope, which is a root coroutine.

https://github.com/Kotlin/kotlinx.coroutines/issues?q=uncaughtexceptionsbeforetest is an indicator of the popularity of breaking structured concurrency in tests. There are numerous questions and complaints about our error-catching functionality, and these are only the questions from people who both use the problematic pattern and found it undesirable to observe errors hidden by it. The tests that were saved by our error-catching are probably much more numerous. In my view, kotlinx.coroutines can't realistically just allow these errors to go unnoticed.

[OliverO2]

Sharing a test dispatcher/scheduler

I don't understand the need to share test dispatchers among different tests.

What made me think of this is the remark in the docs about the underlying scheduler:

> Several dispatchers can share the same scheduler, in which case their knowledge about the virtual time will be synchronized.

However, I'm not aware of a use case which would require this. So maybe we don't need that.

Each test could get its own, independent test dispatcher, test coroutine scheduler, and TestScope could it not?

If we'd look at completely isolated (simple unit) tests only: Yes, that would work. But with TestBalloon, we must be aware of coroutines outside a single test that nevertheless participate in that test.

Tests using a higher-level coroutine

TestBalloon supports higher-level tests like this one:

val SharedFixtureExample by testSuite {
    val service = testFixture { // This lambda returns the fixture's value
        FakeWeatherService().apply {
            connect(token = "TestToken") // can suspend
        }
    } closeWith { // tears down the fixture object at the end of its lifecycle
        disconnect() // can suspend
    }

    test("Temperature in Hamburg is 21.5 °C") {
        // The `service()` invocation accesses the fixture and returns its value 
        assertEquals(21.5, service().location("Hamburg").temperature)
    }

    test("Temperature in Munich is 25.0 °C") {
        assertEquals(25.0, service().location("Munich").temperature)
    }

} // the fixture's lifecycle ends here

In this case, the shared fixture operates in the coroutine of the enclosing test suite. If tests do not mutate the shared state in a conflicting way, they can safely run in parallel. Such tests could still benefit from delay skipping.

So we cannot assume that all coroutines participating in a test were launched inside that test. Nor do they necessarily finish with that test.

Unhandled exceptions reporting (out-of-scope coroutines)

In my view, kotlinx.coroutines can't realistically just allow these errors to go unnoticed.

I have looked into the message of commit 1245d7edb0ec61848b620691406b79c370d706d5, and I agree. Such errors should be detected as soon as possible, come with as much diagnostic information as possible, and fail the test run.

Since TestBalloon controls the entire test execution, it could cover that better than a TestScope bound to a single test could: TestBalloon could use setUncaughtExceptionHandler from the start, report exceptions – failing test execution – as soon as they occur (including of exceptions happening after the last test), and provide extra information, such as the names of tests in progress when the exception was detected.

Conclusions

It seems that all we need here is a suspending subset of runTest which provides the following:

• testScheduler and delay-skipping,
• timeout handling.

What would better be handled by TestBalloon and should not be overruled by kotlinx.coroutines:

• reporting of unhandled exceptions,
• backgroundScope (background coroutines would better be integrated via fixtures with a cancel invocation in the fixture's tear-down lambda).