quic: use Check instead of FromJust in node_quic.cc#33937
quic: use Check instead of FromJust in node_quic.cc#33937danbev wants to merge 1 commit intonodejs:masterfrom
Conversation
addaleax
left a comment
There was a problem hiding this comment.
LGTM
@danbev Since you’re opening quite a few PRs of that kind … would you be interested in helping us move away from .Check()/.FromJust()/.ToLocalChecked() in places where those calls aren’t valid? So far I’ve mostly been trying to avoid that new code uses those inaccurately, but it would of course be nice to also make this work for code like this, where neither .FromJust() nor .Check() are really correct :)
I'd be happy to. Could you give me an example of where these calls are not valid so I understand the reason for them not being valid? |
|
@danbev I think the description in https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion might be helpful? The tl;dr is that anything that potentially runs JS code, including built-in V8 JS code or getters/setters on built-in object’s prototypes, can fail and thus result in a crash. For example, in this particular case: $ node -e 'Object.defineProperty(Object.prototype, "sessionConfig", {set() { throw new Error(); }}); net.createQuicSocket()'
FATAL ERROR: v8::FromJust Maybe value is Nothing.
[...](I know that these |
|
Re-run of failing node-test-commit-arm-fanned ✔️ |
|
@addaleax Thanks, I'll try to take a closer look this week. |
PR-URL: #33937 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed in 013cd1a. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes