search

coopernurse / node-pool

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

AB - maxWaitingClients should respect `0` value #235

Closed anupbaldawa closed 6 years ago

anupbaldawa commented 6 years ago

if 0 is passed to maxWaitingClients then it should respect the value

callmehiphop commented 6 years ago

A little late, but if the default value is null won't this attempt to parseInt a null value? Seems like we might have wanted typeof value === 'number'

anupbaldawa commented 6 years ago

Actually, maxWaitingClients is never assigned a null value in the code. That is why checking for undefined will work.

But checking for number will be a much better check.

sandfox commented 6 years ago

oopss, clumsy fingers! I usually pull PR's down and manually merge/check them first.

now magically unmerged for moment

As regards number/type checking, in short yes it would be better, but as the existing code is somewhat lack lustre in this area I'm ok with it not being type checked. That said, if you want add some checks, please do.

The PoolOptions could do with a vast tidy up when it comes to checking and coercion but thats definatelty the topic/work for another ticket.

Whilst putting together a basic test for this PR I noticed that setting maxWaitingClients=0 would seem to prevent the pool from ever accepting acquire calls because of this block https://github.com/coopernurse/node-pool/blob/72b31a434c7b05ad879b2ace9830a9aa9fbba002/lib/Pool.js#L438-L446

Is that intentional?

callmehiphop commented 6 years ago

Are the PoolDefaults not used? They appear to initalize maxWaitingClients to null.

sandfox commented 6 years ago

not in the case of maxWaitingClients or acquireTimeoutMillis (for reasons unknown at this point in time, probably just overlooked)

anupbaldawa commented 6 years ago

We want that the pool should not wait if the max sessions are created and all the sessions are used.

anupbaldawa commented 6 years ago

@sandfox I think we should also check if max sessions have been created and change

this._waitingClientsQueue.length >= this._config.maxWaitingClients to this._waitingClientsQueue.length > this._config.maxWaitingClients

I will dig more into the code and create another PR.