POC: Time out pg.connect() call #939
Conversation
| pg.defaults.block = false; | ||
| pg.defaults.poolSize = 1; | ||
|
|
||
| test('pool#connect acquire errors if the pool is full', function() { |
There was a problem hiding this comment.
One of these tests introduces an unnecessarily long timeout, maybe 2 seconds. I'm not super familiar with how the test runner knows when tests are done or I would address it, was hoping you'd know :(
There was a problem hiding this comment.
Also - there are more tests around the generic-pool implementation then there are here, this just covers the common cases.
kevinburkeshyp
force-pushed
the
timeout
branch
from
0a1a1b2
to
1d5a07f
Compare
| @@ -4,10 +4,10 @@ connectionString=postgres:// | |||
|
|
|||
| params := $(connectionString) | |||
|
|
|||
| node-command := xargs -n 1 -I file node file $(params) | |||
| node-command := TZ=GMT xargs -n 1 -I file node file $(params) | |||
There was a problem hiding this comment.
+1 nice - sorry I missed that one!
|
I'm a big +1 on these features! Thanks for putting in the time to implement these. I'm on a cruise right now 🚢 with very very bad internet speeds so apologies for taking a while to get to this. As far as the test runner is concerned its very primitive because node-postgres was written before most of the now standard test frameworks for node existed, so I rolled my own with makefiles. Timeouts are handled by the test suite hanging indefinitely - so you can use as short of timeouts as you want. Once I'm back on Monday I'll have time to finish reviewing this - there are a few utility methods I monkey-patched on to the |
|
Hey @kevinburkeshyp thanks again for putting this together! Once the PR is merged for |
|
I hope this is merged soon ;) |
|
Hey @kevinburkeshyp did the changes ever land upstream in generic pool allowing for a timeout before failing the connection? |
|
no, the maintainer has said for 3 months now he's going to look at it, but no progress |
|
Ah - sorry about that! That maintainer sounds about as slow as me. 😬 Once its on npm I'll merge this ASAP. |
|
Can we not finish this without that maintainer? He may as well never come back at this point... |
|
If you fork generic-pool and push it to a new package on npm & then add that package as a dependency that will unblock this - I don't think that is the ideal solution, but it would work. This would also need to be cleaned up so it would once again cleanly merge into master. Then we're good to go! 👏 |
|
Sounds good! Will let you know! |
|
Submitted as #1006! Closing. |
Currently if you call pg.connect(), the call will block indefinitely until a connection becomes available. In many cases, if a connection is not available after some period of time, it's preferable to return an error (and call control) to the client, instead of tying up resources forever. Blocking on resource checkout also makes it easier for clients to deadlock - recently at Shyp, we had a situation where a row got locked and the thread that could unlock it was blocked waiting for a connection to become available, leading to deadlock. In that situation, it would be better to abort the checkout, which would have errored, but also broken the deadlock. Add a new setting to defaults: `acquireTimeout`, which will wait for `acquireTimeout` milliseconds before giving up and returning an error. If the value is undefined (the default), `node-postgres` will continue to wait indefinitely for a connection to become available. This builds on a pull request against `generic-pool`, support options.timeout: coopernurse/node-pool#127. Review has been slow going, so I published a new package with that change as `generic-pool-timeout`, and updated the reference in this codebase. Adds semicolons in many places that omitted them and fixes several typos. I'm happy to pull those out into a different commit. Sets the TZ=GMT environment variable before running the tests; without this value set, and with a Postgres server set to the America/Los_Angeles timezone, a timezone test failed. Fixes brianc#782 and brianc#805. Will help alleviate brianc#902. May help with brianc#397.
Currently if you call pg.connect(), the call will block indefinitely until a
connection becomes available. In many cases, if a connection is not available
after some period of time, it's preferable to return an error (and call
control) to the client, instead of tying up resources forever.
Blocking on resource checkout also makes it easier for clients to deadlock -
recently at Shyp, we had a situation where a row got locked and the thread
that could unlock it was blocked waiting for a connection to become available,
leading to deadlock. In that situation, it would be better to abort the
checkout, which would have errored, but also broken the deadlock.
Add two new settings to defaults:
block, which, if false, will immediatelyreturn an error if the pool is full when you attempt to check out a new
connection. Also adds
acquireTimeout, which will wait foracquireTimeoutmilliseconds before giving up and returning an error.
This builds on two pull requests against
generic-pool:For the moment I'm pointing
generic-poolat a branch that incorporatesboth of these commits. I'm marking this as a proof-of-concept until those go
through, which hopefully they will soon. I'd also like feedback on the
API.
Adds semicolons in many places that omitted them and fixes several typos. I'm
happy to pull those out into a different commit.
Sets the TZ=GMT environment variable before running the tests; without this
value set, and with a Postgres server set to the America/Los_Angeles timezone,
a timezone test failed.
Fixes #782 and #805. Will help alleviate #902. May help with #397.