New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set a timeout on pg.connect #805

Closed
kevinburkeshyp opened this Issue Jun 17, 2015 · 8 comments

Comments

5 participants
@kevinburkeshyp
Copy link
Contributor

kevinburkeshyp commented Jun 17, 2015

Hi,
Thanks for this library, it's great.

Currently if I call pg.connect but my pool is full, node-postgres will block until a connection becomes available. Is there a way to set a timeout on a connection attempt? I would rather not block indefinitely if the pool is full.

Something like pg.connect({timeout: 500}, function(err) { console.log(err); }

I can wrap it in a setTimeout, but do I need to do cleanup/is there a way to cancel a connection attempt?

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Jun 17, 2015

Ugh, looks like this is a known issue in the pool implementation: coopernurse/node-pool#85

@vitaly-t

This comment has been minimized.

Copy link
Contributor

vitaly-t commented Jun 19, 2015

If you use the pool correctly, you won't run out of connections.

By default the library supports 10 connections in the pool. If you release the connection properly after you're done using it, then you won't run out of the connections.

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Jun 19, 2015

Hi Vitaly,
Thanks for your feedback. There are two reasons you could run into this:

  • you're checking out more than 10 connections at a time, or some anomalous event causes load on your system that causes the number of connections to increase beyond the number of connections your database can handle at any one time.
  • you are not releasing the connection in a place that you should be. it can be tricky, and no one's perfect! this is what actually bit me and turned me on to this issue - my tests were blocking and i couldn't figure out why.

in either situation, it's good to be defensive; rather than tying up memory and FD's waiting for a connection that may never materialize, I would rather bail early and return an error to the caller. this pattern's used by netflix, for example, see here: https://github.com/Netflix/Hystrix/wiki/How-it-Works#isolation

@vitaly-t

This comment has been minimized.

Copy link
Contributor

vitaly-t commented Jun 19, 2015

Most likely it is the second case:

you are not releasing the connection in a place that you should be

Managing connections can be tricky. It is impossible to know without the code analysis.

I stopped using connections directly ever since I wrote pg-promise. Maybe it can help you too ;)

kevinburkeshyp pushed a commit to Shyp/node-postgres that referenced this issue Feb 17, 2016

Kevin Burke
POC: Time out pg.connect() call
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 immediately
return an error if the pool is full when you attempt to check out a new
connection. Also adds `acquireTimeout`, which will wait for `acquireTimeout`
milliseconds before giving up and returning an error.

This builds on two pull requests against `generic-pool`:

- Support options.block: coopernurse/node-pool#125
- Support options.timeout: coopernurse/node-pool#127

For the moment I'm pointing `generic-pool` at a branch that incorporates
both 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 brianc#782 and brianc#805. Will help alleviate brianc#902. May help with brianc#397.
@ensonik

This comment has been minimized.

Copy link

ensonik commented Feb 19, 2016

@vitaly-t Hi Vitaly, @kevinburkeshyp point is that a system shouldn't become unresponsive or crash when another external system is mis-behaving. By not having a connection timeout, my application will fail miserably because of something that's out of my control (for example, if the posgresql server is slow).

With the timeout option I can simply return an error to the client after half a second if I chose.

This concept and problem area exists wether I use pg-promise or not.

@vitaly-t

This comment has been minimized.

Copy link
Contributor

vitaly-t commented Feb 20, 2016

@ensonik what you are describing is a connectivity issue. Regardless of which library you are using, it shouldn't be responsibility of the library to handle such issues, it is responsibility of your application.

And since all IO in Node.js is asynchronous, you can easily solve this through a simple timeout.

Initiate each connection+command inside a timeout, and inside the timeout check whether the request has finished or not, and provide the response accordingly. Such logic is easy to wrap into a promise or a simple function.

And in case of pg-promise, some promise libraries already support timeout as a promise operation, like Bluebird does: http://bluebirdjs.com/docs/api/timeout.html, which means it's even easier.

kevinburkeshyp pushed a commit to Shyp/node-postgres that referenced this issue May 27, 2016

Kevin Burke
Add option to time out pg.connect() call
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.
@davepacheco

This comment has been minimized.

Copy link

davepacheco commented Jul 22, 2016

We've run into a similar problem with a command-line tool that queries several different PostgreSQL instances. When we attempt to query an instance that whose server is down, or which is behind a network partition, the program ends up hanging for a long time because the client is still trying to connect to the down server. The CLI tool actually handles this by applying its own timeout and reporting the problem. The problem is that there's no way for the program to cancel the connection (i.e., close the TCP socket), and as a result, the Node program keeps running until the connection ultimately fails. (I know we can call process.exit(), but this would just be papering over the problem.)

If the Client class provided a close() method that just closed the underlying socket (and stopped emitting other events), that would allow callers to implement their own connection timeout.

@charmander

This comment has been minimized.

Copy link
Collaborator

charmander commented Aug 6, 2017

Connection timeouts have been implemented as connectionTimeoutMillis; for applying that to pool size limits, see #1390.

@charmander charmander closed this Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment