-
Notifications
You must be signed in to change notification settings - Fork 100
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
Prevent panic: Custom error class must have a builder registered: Uncaught null #730
base: main
Are you sure you want to change the base?
Conversation
a0c5aab
to
258da57
Compare
Rebased |
@mlafeldt this seems reasonable, could you add a unit test that exercises this code path? |
Return the underlying error message instead of panicking with: "Custom error class must have a builder registered: Uncaught null".
258da57
to
5f2a1a5
Compare
@bartlomieju I'm trying. :) However, as mentioned, I've yet to understand what's going on exactly. The |
Interesting, any chance you could try to reproduce similar setup? It might be easier to do so in the |
Thanks for the tip. Will take another look. My main question right now is why Lines 145 to 157 in c106dd1
|
I'm not entirely sure, it might be a bug in |
@mlafeldt sorry for a slow turn-around. Do you have a test case for this particular panic? If so, can you please link it and I'll look into the problem. I'm really not a fan of matching on string repr of the error to change behavior (even though we already do it in a few places 😬). |
Note: This isn't so much a fix (yet) but a starting point for discussion.
We've been running into a panic in
to_v8_error
:Custom error class must have a builder registered: Uncaught null
. This panic has the unfortunate side effect of being unrecoverable, taking down one of our production services.Here's a stack trace from lldb:
For context: We've implemented ops that communicate with another process via gRPC. If that other process disconnects for whatever reason, the panic is (sometimes?) triggered. In that case
to_v8_error
receives this error from tonic:And the JsError inside the
has_caught
block looks like this:As you can see in the diff, I've added a workaround that works for us but is likely not a proper fix.
I honestly don't know what "custom error class" means here or why it only fails in this particular case.
This problem has existed since at least Deno v1.37, with the difference that the affected ops now require to be marked as reentrant.
I can provide more info if needed. ✌️