Fix two races

[johanneswuerbach]

Fix two races

1. can occur after any connection failure as another _pulseQueue is never started and pending items added during the connect won't be processed

2. occurs when a formerly queued client is processed and finds a free pool slot (e.g. because a previous client errored), but during client.connect times out itself.

[kibertoad]

@johanneswuerbach Tests are failing...

[johanneswuerbach]

Added a fix for the second race.

[charmander]

Can you explain the second one a little more? I’ve copied the simpler first one into a pull request at #111 in case it can go in earlier.

[johanneswuerbach]

@charmander the issue occurs when you use connectionTimeoutMillis and in the following situation:

1. connect call A starts connecting to the DB
2. connect call B received between A and successful DB connection is being queued
3. the db connection of call A fails with an error
4. connect call B is pulsed in, finds a free-slot and attempts a database connection
5. connect call B is timed out by connectionTimeoutMillis and a failure returned to the client
6. the db connection of B is ready and the callback of connect call B called a 2nd time

My fix now creates a pendingItem object, which remembers the callback + timed out state and directly adds the new client to the idle queue in step 6 when the pending item already timed out.

[johanneswuerbach]

I found re-entering into "full" connect here a bit confusing as we already know that we need to create a new client and can never add the waiter to the pending queue again as far as I can see.

Now this makes it more clear by using the newClient method directly here and in connect, wdyt?

[charmander]

Thanks for the explanation!

[vitaly-t]

When will we see this merged?

[johanneswuerbach]

@brianc any chance you could have a look? Happy to rebase, but I would prefer a general 👍 / 👎 first.

[brianc]

I think this looks good! Thanks for the explanation and PR! If you rebase I'll npm-link it into pg and make sure the tests pass over there as well (they should, they don't exercise the pool as much as these do, but wanna be sure) and if everything looks good I'll release a new patch version.

[johanneswuerbach]

@brianc done 👍

[johanneswuerbach]

Let me also know, if you think #110 is worth-while proceeding :-)

[brianc]

K - tested this w/ node-postgres. All is in order. Will release a new patch version shortly. Thanks again!

[vitaly-t]

@brianc Christmas-shortly by any chance? 😄 Throw it under the Christmas tree, will ya? 😄

[brianc]

Ah crap i totally forgot to release this after i tested. Will do! Sorry!

[vitaly-t]

When will it be? :)

[rdlowrey]

bump for great justice.

Really appreciate all the hard work on the library -- this one's definitely impacting us in production. New tagged release would be 💯

Thank you!

[brianc]

yeah...sorry again. Was out of town. release [email protected]