-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Propagate exception to wasm-js and js in propagateExceptionFinalResort
#4472
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
Changes from 5 commits
51cde3a
be4173a
1f78da0
8d7c1ec
4a7845d
7dbce2c
b21dd05
485db2c
ed9d26b
0e480cd
6c1098f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
package kotlinx.coroutines.internal | ||
|
||
import kotlinx.coroutines.* | ||
import kotlin.js.unsafeCast | ||
|
||
internal actual fun propagateExceptionFinalResort(exception: Throwable) { | ||
// log exception | ||
console.error(exception.toString()) | ||
} | ||
internal actual external interface JsAny | ||
|
||
internal actual fun Throwable.toJsException(): JsAny = this.unsafeCast<JsAny>() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package kotlinx.coroutines | ||
|
||
import kotlinx.coroutines.testing.* | ||
import kotlin.js.* | ||
import kotlin.test.* | ||
|
||
class PropagateExceptionFinalResortTest : TestBase() { | ||
@BeforeTest | ||
fun removeListeners() { | ||
// Remove a Node.js's internal listener, which prints the exception to stdout. | ||
js(""" | ||
globalThis.originalListeners = process.listeners('uncaughtException'); | ||
process.removeAllListeners('uncaughtException'); | ||
""") | ||
} | ||
|
||
@AfterTest | ||
fun restoreListeners() { | ||
js(""" | ||
if (globalThis.originalListeners) { | ||
process.removeAllListeners('uncaughtException'); | ||
globalThis.originalListeners.forEach(function(listener) { | ||
process.on('uncaughtException', listener); | ||
}); | ||
} | ||
""") | ||
} | ||
|
||
/* | ||
* Test that `propagateExceptionFinalResort` re-throws the exception on JS. | ||
* | ||
* It is checked by setting up an exception handler within JS. | ||
*/ | ||
@Test | ||
fun testThrows() = runTest { | ||
js(""" | ||
globalThis.exceptionCaught = false; | ||
process.on('uncaughtException', function(e) { | ||
globalThis.exceptionCaught = true; | ||
}); | ||
""") | ||
val job = GlobalScope.launch { | ||
throw IllegalStateException("My ISE") | ||
} | ||
job.join() | ||
delay(1) // Let the exception be re-thrown and handled. | ||
|
||
val exceptionCaught = js("globalThis.exceptionCaught") as Boolean | ||
assertTrue(exceptionCaught) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package kotlinx.coroutines.internal | ||
|
||
import kotlinx.coroutines.* | ||
|
||
internal expect interface JsAny | ||
|
||
internal expect fun Throwable.toJsException(): JsAny | ||
|
||
/* | ||
* Schedule an exception to be thrown inside JS or Wasm/JS event loop, | ||
* rather than in the current execution branch. | ||
*/ | ||
internal fun throwAsync(e: JsAny): Unit = js("setTimeout(function () { throw e }, 0)") | ||
dkhalanskyjb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
internal actual fun propagateExceptionFinalResort(exception: Throwable) { | ||
throwAsync(exception.toJsException()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @murfel @dkhalanskyjb can we throw an exception right from here? May it break anything inside kx-coroutines? In my demo project, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this method is not allowed to fail. It's called deep inside the coroutine internals, and if it throws anything, things will break. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From kotlin 2.2.20-Beta2, in kotlin code, you can just throw a kotlin exception, and everything else will be done by kotlin runtime/compiler, including unwrapping
But this way, users using old toolchain will get a We can try kotlin version at runtime and use a fallback for older versions, but should we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can replace this with just |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,16 @@ | ||
package kotlinx.coroutines.internal | ||
|
||
import kotlinx.coroutines.* | ||
internal actual typealias JsAny = kotlin.js.JsAny | ||
|
||
internal actual fun propagateExceptionFinalResort(exception: Throwable) { | ||
// log exception | ||
console.error(exception.toString()) | ||
} | ||
internal actual fun Throwable.toJsException(): JsAny = | ||
toJsError(message, this::class.simpleName, stackTraceToString()) | ||
|
||
internal fun toJsError(message: String?, className: String?, stack: String?): JsAny { | ||
js(""" | ||
const error = new Error(); | ||
error.message = message; | ||
error.name = className; | ||
error.stack = stack; | ||
return error; | ||
""") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package kotlinx.coroutines | ||
|
||
import kotlinx.coroutines.testing.TestBase | ||
import kotlin.test.* | ||
|
||
class PropagateExceptionFinalResortTest : TestBase() { | ||
/* | ||
* Test that `propagateExceptionFinalResort` re-throws the exception on Wasm/JS. | ||
* | ||
* It is checked by setting up an exception handler within Wasm/JS. | ||
*/ | ||
@Test | ||
fun testThrows() = runTest { | ||
addUncaughtExceptionHandler() | ||
murfel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
val job = GlobalScope.launch { | ||
throw IllegalStateException("My ISE") | ||
} | ||
job.join() | ||
delay(1) // Let the exception be re-thrown and handled. | ||
murfel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
assertTrue(exceptionCaught()) | ||
} | ||
} | ||
|
||
private fun addUncaughtExceptionHandler() { | ||
js(""" | ||
globalThis.exceptionCaught = false; | ||
process.on('uncaughtException', function(e) { | ||
globalThis.exceptionCaught = true; | ||
}); | ||
""") | ||
} | ||
|
||
private fun exceptionCaught(): Boolean = js("globalThis.exceptionCaught") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect a more descriptive name, like
testHandlingUncaughtExceptionsInNodeJs
. Maybe even a documentation comment explaining what exactly we are checking here. Given how much JS code there is in this file, it's not trivial to figure out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like test name is too short to include a very specific descriptive name, so I've only added a comment. Do you want me to rename the test name anyway into the next best thing? (I assume with the goal of having a better name should this test fail.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, having a rough idea of what failed when you see the list of failed tests after introducing a change is the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that all the info is in the class name + path (for platform), and naming this accurately would be duplicating those. Had we had a dozen tests, we wouldn't be naming them so verbosely. I've done it anyways for now.