search

jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.15k stars 1.73k forks source link

emscripten: build with NODEJS_CATCH_REJECTION=0 #1025

Closed svvac closed 3 years ago

svvac commented 3 years ago

This stops emscripten from adding a unhandledRejection handler on the global process object that forcefully exits the process.

This allows fixing emscripten.js#253 because we no longer need to remove these listeners to compensate after the fact.

svvac commented 3 years ago

The CI issue seems bogus. It looks like some kind of connectivity problem.

git clone -q https://github.com/jedisct1/libsodium.git C:\projects\libsodium
remote: Not Found
fatal: repository 'https://github.com/jedisct1/libsodium.git/' not found
Command exited with code 128
svvac commented 3 years ago

Also, regarding possible side-effects from this option, it appears that its sole use is to control the presence of the offending handler, according to GitHub search in the emscripten repository.

Now the question might be what the consequences of not having the process exit might be in the case of libsodium. I found some (unconclusive) discussion around that topic in emscripten#11532.

From what I understand, it is mostly here to enforce error handling in node if case the --unhandled-rejections=strict is not enabled in the engine, which I think is the responsibility of the user of the library. And as a library, I don't think libsodium is intended to crash the process on its end. This is substantiated by the fact that libsodium.js actively attempts to work around this behavior on its end.

Sorry for the noise, thinking out loud here. I don't like fiddling around in critical code I don't understand without explaining my (limited) reasoning for review.

jedisct1 commented 3 years ago

Can you check whether it actually fixes the issue you were reporting?

Since the changes would need to go to stable, we need to be very careful about possibly breaking changes.

svvac commented 3 years ago

My problem was with libsodium.js removing my process handlers to work around the forced process.exit() calls added by emscripten. This is gone after libsodium.js#265 (quick check with grep in a comment on that PR). I do not know of a reproduction test for checking the behavior of the thrown exceptions, but I explained above why I think this should not break things.