Improve handling of DispatchException in an undispatched case #4272

qwwdfsad · 2024-11-11T11:58:59Z

No description provided.

qwwdfsad · 2024-11-11T12:13:12Z

Looking at #4209 (comment), I have yet to see an elegant, generalized solution to our problem that is neither a breaking change nor an obvious contract breach

dkhalanskyjb · 2025-02-17T11:15:58Z

kotlinx-coroutines-core/common/src/intrinsics/Undispatched.kt

+
+private fun ScopeCoroutine<*>.dispatchException(e: DispatchException) {
+    makeCompleting(CompletedExceptionally(e.cause))
+    throw throw recoverStackTrace(e.cause, uCont)


One throw should be enough.

I wanted to be extra-sure the exception is thrown

dkhalanskyjb · 2025-02-17T11:19:09Z

kotlinx-coroutines-core/common/src/intrinsics/Undispatched.kt

+    return cause !is TimeoutCancellationException || cause.coroutine !== this
+}
+
+private fun ScopeCoroutine<*>.dispatchException(e: DispatchException) {


I think this function should be inlined. It's not obvious that it calls makeCompleting.

I am, in general, for the opposite approach -- get rid of all "very uncommon" cases from a code path that is supposed to be followed. I renamed the function and changed return type to be Nothing for the readability sake

Do you have examples of where this approach improves code clarity? My intuition is that this only works in backend-like applications where you let all errors bubble up as exceptions and then just restart the request.

My approach 95% of the time is that a function must be a non-leaky abstraction: it hides the details that are not important to you, but doesn't hide the details that are. dispatchExceptionAndMakeCompleting is not an abstraction, the name is a (non-compiler-verified) enumeration of what it does. If the idea is to use as little space as possible for this codepath, we can join the two statements with ;.

dkhalanskyjb · 2025-02-17T11:21:58Z

kotlinx-coroutines-core/jvm/test/FailFastOnStartTest.kt

+import org.junit.runners.model.TestTimedOutException
+import java.util.concurrent.TimeoutException


Are these imports used?

Nope. Fixed

dkhalanskyjb · 2025-02-17T11:26:00Z

kotlinx-coroutines-core/common/src/intrinsics/Undispatched.kt

+}
+
+private fun ScopeCoroutine<*>.dispatchException(e: DispatchException) {
+    makeCompleting(CompletedExceptionally(e.cause))


Why not makeCompletingOnce, to highlight the aspects of this codepath that are different from the happy path?

I would like this path to be as robust as possible. There might be obscure reasons for DispatchException, for example a dispatcher sent coroutine to execute and then failed in the post-send code-path.
makeCompletingOnce can throw, and I would like to avoid extra try-catches here to preserve the contextual information (i.e. the suppressed dispatch exception)

dkhalanskyjb · 2025-02-17T11:27:47Z

kotlinx-coroutines-core/jvm/test/FailFastOnStartTest.kt

+    fun testFlowOn()  {
+        // See #4142, this test ensures that `coroutineScope { produce(failingDispatcher, ATOMIC) }`
+        // rethrows an exception. It does not help with the completion of such a coroutine though.
+        // Hack to avoid waiting for test completion


Where's the hack?

Made the comment more straightforward

kotlinx-coroutines-core/jvm/test/FailFastOnStartTest.kt

…dispatched mode

…t immediately Otherwise, in the face of malfunctioning dispatcher, there might be no trace of any error, making it extremely hard to debug. The root cause should be addressed by #4209 Fixes #4142

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [org.jetbrains.kotlinx:kotlinx-coroutines-test](https://github.com/Kotlin/kotlinx.coroutines) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.10.1` -> `1.10.2` | | [org.jetbrains.kotlinx:kotlinx-coroutines-slf4j](https://github.com/Kotlin/kotlinx.coroutines) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.10.1` -> `1.10.2` | | [org.jetbrains.kotlinx:kotlinx-coroutines-core](https://github.com/Kotlin/kotlinx.coroutines) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `1.10.1` -> `1.10.2` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.16` -> `2.31.17` | --- ### Release Notes <details> <summary>Kotlin/kotlinx.coroutines (org.jetbrains.kotlinx:kotlinx-coroutines-test)</summary> ### [`v1.10.2`](https://github.com/Kotlin/kotlinx.coroutines/blob/HEAD/CHANGES.md#Version-1102) [Compare Source](Kotlin/kotlinx.coroutines@1.10.1...1.10.2) - Fixed the `kotlinx-coroutines-debug` JAR file including the `module-info.class` file twice, resulting in failures in various tooling ([#4314](Kotlin/kotlinx.coroutines#4314)). Thanks, [@RyuNen344](https://github.com/RyuNen344)! - Fixed `Flow.stateIn` hanging when the scope is cancelled in advance or the flow is empty ([#4322](Kotlin/kotlinx.coroutines#4322)). Thanks, [@francescotescari](https://github.com/francescotescari)! - Improved handling of dispatcher failures in `.limitedParallelism` ([#4330](Kotlin/kotlinx.coroutines#4330)) and during flow collection ([#4272](Kotlin/kotlinx.coroutines#4272)). - Fixed `runBlocking` failing to run its coroutine to completion in some cases if its JVM thread got interrupted ([#4399](Kotlin/kotlinx.coroutines#4399)). - Small tweaks, fixes, and documentation improvements. </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 3449fc325ac14573dff5b670e407e99eab23757b

qwwdfsad requested a review from dkhalanskyjb November 11, 2024 11:59

qwwdfsad changed the title ~~Dispatch exception path~~ Improve handling of DispatchException in an undispatched case Nov 11, 2024

dkhalanskyjb requested changes Feb 17, 2025

View reviewed changes

qwwdfsad added 3 commits February 18, 2025 12:08

Properly rename test that is responsible for fast-path failing for un…

589e75e

…dispatched mode

Simplify, document and reduce the binary size of Undispatched.kt

b96c9a2

Rethrow dispatcher exceptions during the undispatched coroutines star…

8389768

…t immediately Otherwise, in the face of malfunctioning dispatcher, there might be no trace of any error, making it extremely hard to debug. The root cause should be addressed by #4209 Fixes #4142

qwwdfsad force-pushed the dispatch-exception-path branch from f8503e4 to 8389768 Compare February 18, 2025 11:08

~cleanup

f7acf1e

qwwdfsad requested a review from dkhalanskyjb February 18, 2025 11:19

dkhalanskyjb approved these changes Mar 10, 2025

View reviewed changes

dkhalanskyjb merged commit 3f3f4f7 into develop Mar 10, 2025
1 check passed

dkhalanskyjb deleted the dispatch-exception-path branch March 10, 2025 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of DispatchException in an undispatched case #4272

Improve handling of DispatchException in an undispatched case #4272

qwwdfsad commented Nov 11, 2024

qwwdfsad commented Nov 11, 2024 •

edited

Loading

dkhalanskyjb Feb 17, 2025

qwwdfsad Feb 18, 2025

dkhalanskyjb Feb 17, 2025

qwwdfsad Feb 18, 2025

dkhalanskyjb Feb 18, 2025

dkhalanskyjb Feb 17, 2025

qwwdfsad Feb 18, 2025

dkhalanskyjb Feb 17, 2025

qwwdfsad Feb 18, 2025

dkhalanskyjb Feb 17, 2025

qwwdfsad Feb 18, 2025

		import org.junit.runners.model.TestTimedOutException
		import java.util.concurrent.TimeoutException

Improve handling of DispatchException in an undispatched case #4272

Improve handling of DispatchException in an undispatched case #4272

Conversation

qwwdfsad commented Nov 11, 2024

qwwdfsad commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad commented Nov 11, 2024 •

edited

Loading