search

coopernurse / node-pool

Generic resource pooling for node.js
2.38k stars 259 forks source link

drain should be async always #139

Closed thefourtheye closed 7 years ago

thefourtheye commented 8 years ago

The drain function should always invoke the callback asynchronously. As it is, if the the first two conditions are not satisfied, then the callback will be executed synchronously.

thefourtheye commented 8 years ago

Bump! cc @coopernurse @sandfox

thefourtheye commented 7 years ago

Ping!

sandfox commented 7 years ago

sorry, this one has just passed me by completely. Looks good, but could swap the nextTick for a setTimeout(/* */, 0). nextTick can lead to I/O starvation (although pretty unlikely here i admit) and it also has changeable behaviour across different node versions. Whilst setTimeout isn't SUPER efficient, it shouldn't matter in this case, and we can't guarantee that we're running in a version of nodejs that has setImmediate. (Although feel free to make a little wrapper function that detects if we do have it and then uses else fallsback to setTimeout)

Also - As the master branch is now v3 of the library, I'll need to fork off of a brach for the v2 so could you target the PR there (I'll update this issue with it once I've done it)

sandfox commented 7 years ago

V2 branch here -> https://github.com/coopernurse/node-pool/tree/v2.4

thefourtheye commented 7 years ago

Closing in favor of #156