Skip to content
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

Build without greedy uncaughtException handler #262

Closed
danielbarela opened this issue Mar 9, 2019 · 2 comments · Fixed by #255
Closed

Build without greedy uncaughtException handler #262

danielbarela opened this issue Mar 9, 2019 · 2 comments · Fixed by #255

Comments

@danielbarela
Copy link

This is related to #173.

I wrote up what is happening here: https://github.com/danielbarela/sqljs-uncaughtexception

This issue in emscripten indicates that the library can be built without the greedy exception handler:
emscripten-core/emscripten#5957

Can this option be added to the sql.js build and republished?

@Taytay
Copy link
Contributor

Taytay commented Apr 26, 2019

Oh wow! I just read about this behavior and came across this Emscripten issue: emscripten-core/emscripten#2629

That's where the code originally got added. I've been reading about this, and my current thought is that if we were compiling an executable, it would be appropriate to have this global exception handling added so that Emscripten can manage exiting from the process, as it apparently wants to do. However, for a library like Sql.js, it feels inappropriate to me to handle global exceptions simply by including the library in your Node app. So, I agree and will fix this in #255. We'll see what the maintainers think.

Taytay added a commit to Taytay/sql.js that referenced this issue Apr 26, 2019
Fixes kripken/sql.js/sql-js#173
Fixes kripken/sql.js/sql-js#262
@lovasoa
Copy link
Member

lovasoa commented Apr 26, 2019

I agree we should not handle global exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants