propagateExceptionFinalResort on web, add reportError - #4631
Conversation
…rtError # Conflicts: # kotlinx-coroutines-core/js/src/internal/CoroutineExceptionHandlerImpl.kt # kotlinx-coroutines-core/jsAndWasmJsShared/src/internal/CoroutineExceptionHandlerImpl.kt # kotlinx-coroutines-core/wasmJs/src/internal/CoroutineExceptionHandlerImpl.kt
|
My common request - avoid |
|
Yes, I remember our discussion, but I'd like to merge this as is for now, since it's been a long-standing issue. Later we could apply a refactoring to all our |
|
Please describe at least a manual testing procedure for this (it would be challenging to test automatically, since we don't have browser tests enabled, that much is clear). Figuring out from scratch any way to check the validity of this code every time we need to may take a lot of effort, given that Bun and Deno are not part of our daily lives. |
| error.stack = stack; | ||
| setTimeout(function () { throw error }, 0); | ||
| if (typeof globalThis.reportError === 'function') { | ||
| // Modern browsers and some non-browser JS runtimes (Deno, Bun) |
There was a problem hiding this comment.
It's unclear why we should choose it over setTimeout supported everywhere. If there are any reasons to use reportError if it is available, it's worth explicitly mentioning them here.
There was a problem hiding this comment.
The benefit should be clear if we look at this example:
suspend fun main() {
registerErrorHandler()
supervisorScope {
launch {
val suppressed = IllegalStateException("suppressed")
val exception = Exception("test")
exception.addSuppressed(suppressed)
throw exception
}
}
println("Hello, Kotlin/JS!")
var i = 0
while (true) { // HANG FOREVER!!!
++i
}
}Trying to open this in a browser will lead to a hang. The browser will suggest stopping the script execution on the page. After you do, there's no error in sight, because we terminated the JS runtime before the setTimeout lambda managed to run.
On the other hand, reportError reports the error reliably.
For hanging pages, I'd want to learn of the critical errors that happened right before the hang, so reportError is a clear improvement.
|
JFTR, there was a way to configure Wasm target to use Deno (https://github.com/Kotlin/kotlin-wasm-wasi-template/blob/ilgonmic/prototype-runtimes/buildSrc/src/main/kotlin/deno/declare.kt), perhaps, a similar trick could be applied to JS too. |
|
Manual testing for browser I prepped a small demo. Open in IDEA and click run in the run configuration dropdown, a page in browser will open, right-click inspect (in Chrome), go to the console tab, and see the error printed to the console. To test the proposed behaviour, checkout this branch, publish this branch locally with https://github.com/Kotlin/kmp-js-coroutines The console printing style changes significantly (see screenshots). Currently, you can clearly see that it came from Another behaviour difference is that currently an error event listener wouldn't intercept the exception, but with Current behaviour (uncollapsed error dropdown): Proposed behaviour with |
|
@fzhinkin would you like to discuss or add something else? If not, I'll merge. |
…porting mechanism, not a novelty api
4 participants
As requested by @turansky in #4451