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

bcrypt crashes in child process #709

Closed
Suvitruf opened this issue Mar 8, 2019 · 22 comments
Closed

bcrypt crashes in child process #709

Suvitruf opened this issue Mar 8, 2019 · 22 comments
Assignees
Labels

Comments

@Suvitruf
Copy link

Suvitruf commented Mar 8, 2019

If you are trying to load bcrypt in child process (worker thread) it will fail with the error:

Error: Module did not self-register.
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:779:18)
    at Module.load (internal/modules/cjs/loader.js:630:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
    at Function.Module._load (internal/modules/cjs/loader.js:562:3)
    at Module.require (internal/modules/cjs/loader.js:667:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (O:\Texts\Sources\my\bcrypt-shild-process\node_modules\bcrypt\bcrypt.js:6:16)
    at Module._compile (internal/modules/cjs/loader.js:738:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:749:10)
    at Module.load (internal/modules/cjs/loader.js:630:32)

Info:

  • Node v11.10.0
  • bcrypt v3.0.4

I've created sample project for this issue: https://github.com/Suvitruf/bcrypt-child-process-crash-proof

@recrsn
Copy link
Collaborator

recrsn commented Mar 8, 2019

What would be the output if it runs?

Can you try experimenting with the n-api version of the module? npm i bcrypt@napi

@Suvitruf
Copy link
Author

Suvitruf commented Mar 8, 2019

I've updated my repo. The output should be:

I'm ok, because I'm alpha main
same
I'm sad, because I can't load bcrypt
same

Same problem with n-api version.

@recrsn
Copy link
Collaborator

recrsn commented Mar 8, 2019

child_process is not same as worker_thread. I guess, we have some issues when multiple threads try to access native libraries.

In my system (Ubuntu 18.04, Node 11.11.0), napi crashes without any error message

Edit: actually does not crash and instead works fine.

09:07:49 PM IST | amitosh ❱ bcrypt-child-process-crash-proof ❱
❱ node index.js 
I'm ok, because I'm alpha main
same
I'm sad, because I can't load bcrypt
same

@Suvitruf
Copy link
Author

Suvitruf commented Mar 8, 2019

I'm running it on Windows 7.

@recrsn
Copy link
Collaborator

recrsn commented Mar 8, 2019

Here's my code after NAPI modifications: https://github.com/agathver/bcrypt-child-process-crash-proof/tree/napi

I would need to catch hold of a Windows machine to test this.

I saw worker_threads are still marked as experimental, and we have warnings about using native modules in electronjs Web workers. Perhaps there may be some bugs in the implementation.

@Suvitruf
Copy link
Author

Suvitruf commented Mar 8, 2019

I saw worker_threads are still marked as experimental

Yeh, but recently they said that it's kinda stable right now: nodejs/node#25361. You don't even need to use --experimental-worker flag anymore.

@recrsn recrsn added the bug label Mar 19, 2019
@NickNaso
Copy link
Contributor

I everyone the problem here is that when you use worker-threads the addon will be loaded multiple times in multiple contexts and to allow this the addon must support multiple initializations.
My opinion is he it's necessary to modify brcypt to be a context aware addon.
@agathver @Suvitruf What do you think about this? Do you agree to apply this change?

@Suvitruf
Copy link
Author

@NickNaso good idea. But I'm not familiar with native modules =/

@recrsn
Copy link
Collaborator

recrsn commented Mar 25, 2019

@NickNaso The module uses no static variables, this change should be trouble free. We need to add a new worker_threads test case too.

@NickNaso
Copy link
Contributor

If it's ok for you I will try to work on this issue and solve the problem.

@recrsn
Copy link
Collaborator

recrsn commented Mar 26, 2019 via email

@recrsn
Copy link
Collaborator

recrsn commented Apr 1, 2019

@NickNaso Let me know if you will be able to work on the issue. It's a minor change. Else I would go ahead and publish the version myself

@NickNaso
Copy link
Contributor

NickNaso commented Apr 1, 2019

@agathver if you want go to work on the issue. Unfortunately I had not time last week.

@khangaridb
Copy link

Any updates on this issue?

@recrsn recrsn self-assigned this Apr 10, 2019
@recrsn
Copy link
Collaborator

recrsn commented Apr 10, 2019

@khangaridb You can use the napi variant of the module. npm i bcrypt@napi

The primary module will be compatible in few days, I did some initial work and it needs some polish before I can publish

@Suvitruf
Copy link
Author

You can use the napi variant of the module. npm i bcrypt@napi

Which version? ^3.0.6-napi doesn't work on Windows.

@recrsn
Copy link
Collaborator

recrsn commented Apr 15, 2019

napi is 3.0.4, it doesn't quite track mainline bcrypt yet

@barry1cassidy
Copy link

I have this same problem on windows 10. Any update?

@yovanoc
Copy link

yovanoc commented Jan 2, 2020

I have this problem too, even with the napi one, any update?

@recrsn
Copy link
Collaborator

recrsn commented Feb 21, 2020

Can you check if v4 solves your problem?

@NickNaso
Copy link
Contributor

@agathver The version 4 is not published on npm, but only available on GitHub.

@NickNaso
Copy link
Contributor

@agathver The version 4 is not published on npm, but only available on GitHub.

Now is on npm Thanks to everybody.

@recrsn recrsn closed this as completed Feb 26, 2020
joelgallant added a commit to launchcodedev/germinator that referenced this issue Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants