Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upFix crypto abort on throwing setter #27157
Conversation
sam-github
requested a review
from addaleax
Apr 9, 2019
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
C++
crypto
labels
Apr 9, 2019
sam-github
changed the title
Fix tls abort on throwing setter
Fix crypto abort on throwing setter
Apr 9, 2019
src/node_crypto.cc Outdated
src/node_crypto.cc Outdated
src/node_crypto.cc Outdated
src/node_crypto.cc Outdated
jasnell
approved these changes
Apr 10, 2019
sam-github
force-pushed the
sam-github:fix-abort-on-throwing-setter
branch
2 times, most recently
from
9686a81
to
cf6e4d2
Apr 11, 2019
This comment has been minimized.
This comment has been minimized.
|
@addaleax made your suggested changes, still this open question:
|
This comment has been minimized.
This comment has been minimized.
|
@sam-github Yeah, I don’t think there’s a definite right or wrong answer. But I think forwarding the setter exception is what would happen if we were to do this in JS land (unless there are explicit |
sam-github
force-pushed the
sam-github:fix-abort-on-throwing-setter
branch
from
cf6e4d2
to
ab0787c
Apr 12, 2019
This comment has been minimized.
This comment has been minimized.
|
@addaleax the analogy with js is not entirely perfect, because the openssl error is not (yet) an exception, but it is an "error". The properties are doced as "exist if they do" (lots of crypto errors don't come from openssl, so don't have the properties), so that isn't a problem, but I'll leave it as it is, throwing the first pending exception. Its a total fringe case, its hard to imagine anyone attaching throwing setters on the global object prototype and expecting deterministic behaviour. |
sam-github
force-pushed the
sam-github:fix-abort-on-throwing-setter
branch
from
ab0787c
to
4b43d49
Apr 12, 2019
This comment has been minimized.
This comment has been minimized.
sam-github
force-pushed the
sam-github:fix-abort-on-throwing-setter
branch
from
4b43d49
to
aff1a64
Apr 12, 2019
sam-github
force-pushed the
sam-github:fix-abort-on-throwing-setter
branch
from
aff1a64
to
89432ac
Apr 14, 2019
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Landed in 69140bc |
sam-github commentedApr 9, 2019
See #26868 (comment)
@addaleax Note there are 2 commits. Actually, I'm not convinced the second commit is correct.
If a C++ API called from js has an openssl error occur, then while creating the error object to throw from C++, it calls into user-land js, and THAT throws, which error should actually suface? With all the Maybe checking, I made it be the "pending" low-level error. That was a useful exercise, it gives me a better understanding what all the Maybe's are about, but I have to say, I'm not sure its what I would expect.
In this case, isn't it the first error, the openssl API one, that we would want thrown? Not the second one?
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes