search

brianc / node-pg-pool

A connection pool for node-postgres
MIT License
180 stars 64 forks source link

Fix two races #109

Closed johanneswuerbach closed 5 years ago

johanneswuerbach commented 5 years ago

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 commented 5 years ago

@johanneswuerbach Tests are failing...

johanneswuerbach commented 5 years ago

Added a fix for the second race.

charmander commented 5 years ago

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 commented 5 years ago

@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.

charmander commented 5 years ago

Thanks for the explanation!

vitaly-t commented 5 years ago

When will we see this merged?

johanneswuerbach commented 5 years ago

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

brianc commented 5 years ago

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 commented 5 years ago

@brianc done 👍

johanneswuerbach commented 5 years ago

Let me also know, if you think https://github.com/brianc/node-pg-pool/pull/110 is worth-while proceeding :-)

brianc commented 5 years ago

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

vitaly-t commented 5 years ago

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

brianc commented 5 years ago

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

vitaly-t commented 5 years ago

When will it be? :)

rdlowrey commented 5 years ago

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 commented 5 years ago

yeah...sorry again. Was out of town. release pg-pool@2.0.6