search

coopernurse / node-pool

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

Fix validateAsync usage #142

Closed felipou closed 7 years ago

felipou commented 8 years ago

When calling acquire multiple times in a row, the async validation would dispense the first object to all callers.

This commit fixes that by removing the object before validating it.

I'm also adding a verification to check if clientCb is valid before using it, because we may reach the validation phase twice even if there is only one waiting client, and therefore we would end up with a null clientCb.

There is another way that we could avoid the clientCb being null: we could dequeue it just before calling doWhileAsync. Since this would happen just after verifying the queue size, we would be sure it wouldn't be null. However, if we then reached the point where a resource needs to be created, we would need to add the clientCb to the queue again before calling _createResource, or we could modify the _createResource to accept an optional argument that is the clientCb.

The downside of the way I did it is that the validateAsync function may called more times than necessary. The downside of re-adding an item to the queue is that it would be strangely reordered (also I didn't think this through to see if it would result in some problem). The downside of modifying _createResource is that we would need to interfere with other internal functions without the need to do so (I also wasn't totally convinced that this would solve the problem without causing trouble elsewhere).

I'll be implementing the other options soon and will post a link to them in the comments here, so that we can better compare then.

felipou commented 8 years ago

This is the commit where I add an argument to the _createResource function: https://github.com/felipou/node-pool/commit/76799898ec1f2f683d56837dc7144eb32e1c79e4

felipou commented 8 years ago

And this is the commit where I re-enqueue the clientCb just before calling _createResource: https://github.com/felipou/node-pool/commit/a6937b2677c46d1292eb9ab888caae725d9ac704 I also had to modify the tests, because with this method, the last caller in a sequence is the first to get an object (since we reverse the queue order).

felipou commented 8 years ago

I noticed that the Travis build failed because of style differences, once we settle on which will be the final solution I will fix these.

felipou commented 8 years ago

There is another solution that I forgot to mention, and it is probably the best solution, but it would require a new function on PriorityQueue to enqueue to the start of the queue instead of the end. Then we could use it to re-enqueue the clientCb just before calling _createResource at the start of queue. I'm going to look more carefully at this option.

felipou commented 8 years ago

I looked at the code and remembered why this isn't an easy solution: we don't have the clientCb priority at the time of re-enqueueing.

jcreigno commented 7 years ago

Hi,

I'm facing exactly the issue you're describing. I'm using node-pool through node-pg-pool, and under heavy load (twice the pool max size) node end up crashing with the following stack : 

clientCb is not a function TypeError: clientCb is not a function
    at /opt/presstalis/node_modules/architect-pg-pool/node_modules/pg/node_modules/pg-pool/node_modules/generic-pool/lib/generic-pool.js:288:13
...

It appeared after adding the validateAsync property in the pool config.

Do you plan to merge this PR soon ? Is there something I can do to help merging it ?

It seems that PR #145 is addressing the similar issue.

felipou commented 7 years ago

I don't have merge permission, and I don't know if this will be merged anytime soon, because I think the owner is focused on finishing v3.

sandfox commented 7 years ago

I've landed a bunch of code based of this on v2.5.1 . It should fix the asyncValidate problems (maybe not completely, but better than it was!)

I took your original PR and re-worked / mashed it up with some bits from v3 of the pool. I followed the decision in v3 that it was ok to end up with "too many" validateAsync calls and any resources that passed validation but had no caller to be given to would be re-added to the pool as if they had been returned / created.

felipou commented 7 years ago

Great! I thought v2 was abandoned, but this will be useful until I finish upgrading to v3!

sandfox commented 7 years ago

I really do want to abandon V2 :-p but I think it's going to take a few more months at least for people to really move over to using v3 (and there are of course wrinkles in v3 that need sorting).