search

shaneMangudi / bcrypt-nodejs

Native implementation of bcrypt for NodeJS
Other
574 stars 69 forks source link

Truly async hashing, compatibility fixes and some minor improvements #12

Closed dcodeIO closed 7 years ago

cmcnulty commented 10 years ago

+1 for this pull - The current progress callback doesn't really work because the tight while loop never gives a chance to update any kind of progress meter.

fpirsch commented 10 years ago

I've forked and fixed this project, and, among other things, the demo shows nice progress meters in action :-) http://fpirsch.github.io/twin-bcrypt/

dcodeIO commented 10 years ago

fyi: https://github.com/dcodeIO/bcrypt.js/issues/10

joshball commented 9 years ago

@dcodeIO @fpirsch Thanks for the PR. Can one of you pull master and update this PR. As well, I would like to see some tests demonstrating that your changes haven't broken anything. I am not that familiar with the code base here, but just trying to push things forward. For instance, your changes to getByte() look ok, but it is significant enough that I would love to test edge cases around the previous catch/throw, etc... I don't want to be the one to allow code in that breaks the lib. ;-)

RichardWright commented 7 years ago

Question - wouldn't this behaviour block the event loop regardless due to the cpu intensive nature of hashing? the extensions within the node.bcrypt.js use threads to stop this.

dcodeIO commented 7 years ago

One could still use a WebWorker with the sync API instead, which I'd recommend. The async functionality just makes sure to yield from the otherwise long blocking operation often enough so that other operations can execute while bcrypt computation is ongoing.

RichardWright commented 7 years ago

Aren't webworkers for the browser? Wouldn't the cluster api be more appropriate?

dcodeIO commented 7 years ago

On node it's not a WebWorker but something otherwise thready, yeah.

RichardWright commented 7 years ago

Cluster is a separate process. I must admit that I am unsure when to use a different process vs using a thread in nodejs.

But any kind of hashing carried out in the same event loop, async or sync cannot be good for performance.

I should do a test to prove this!

dcodeIO commented 7 years ago

As long as it is just one additional process, a child_process should be fine here. If more than that is required then there are a couple real threading modules on npm (like this one with a WebWorker-like API).