/node-postgres

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

Add option to time out pg.connect() call #1006

Closed
wants to merge 1 commit into
base: master
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
  pg@7.0
7 participants
@kevinburke @vitaly-t @brianc @abenhamdine @TooAngel @drmrbrewer @charmander
@kevinburke
Contributor

kevinburke commented May 2, 2016

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 #782 and #805. Will help alleviate #902. May help with #397.

This was referenced May 2, 2016

Closed
Closed
@vitaly-t

This comment has been minimized.

Show comment
Hide comment
@vitaly-t

vitaly-t May 2, 2016

Contributor

Great job! Although I would have called it connectTimeout instead, easier for the first-time reader to grasp the meaning of it, acquire isn't very user-friendly, carries no specific meaning :)
Contributor

vitaly-t commented May 2, 2016 •
edited
Edited 1 time
  • @vitaly-t vitaly-t edited May 2, 2016 (most recent)

Great job! Although I would have called it connectTimeout instead, easier for the first-time reader to grasp the meaning of it, acquire isn't very user-friendly, carries no specific meaning :)
@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke May 2, 2016

Contributor

My worry about connect timeout is it has a really specific meaning, the amount of time before you give up on a CONNECT syscall. In this instance, I'd expect a connectTimeout to indicate the amount of time it took for a socket to be established to the remote database.

see also https://curl.haxx.se/libcurl/c/CURLOPT_CONNECTTIMEOUT.html, http://docs.python-requests.org/en/master/user/advanced/#timeouts.
Contributor

kevinburke commented May 2, 2016

My worry about connect timeout is it has a really specific meaning, the amount of time before you give up on a CONNECT syscall. In this instance, I'd expect a connectTimeout to indicate the amount of time it took for a socket to be established to the remote database.

see also https://curl.haxx.se/libcurl/c/CURLOPT_CONNECTTIMEOUT.html, http://docs.python-requests.org/en/master/user/advanced/#timeouts.
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 #782 and #805. Will help alleviate #902. May help with #397.
e795d17
@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jun 13, 2016

Owner

@kevinburke this is really fabulous - I appreciate you putting the time in here! I've recently returned from a long, dark time of not using node-postgres enough back into the beautiful world of lots of node + postgres work! I actually have had my eye on the issue of require('pg') returning a weird singleton pool thing for a long time so last week I started breaking the pool out into its own module: pg-pool. I have a branch of node-postgres which removes all the pooling code from node-postgres directly & instead leans on this pooling module here. So far the changes are backwards compatible I think, which is neat. Anyways...this is gonna cause some epic levels of rebase problems for this PR unfortunately. If you'd like I can integrate your changes into pg-pool. If I do so mind if I ping you for a review over there?
Owner

brianc commented Jun 13, 2016

@kevinburke this is really fabulous - I appreciate you putting the time in here! I've recently returned from a long, dark time of not using node-postgres enough back into the beautiful world of lots of node + postgres work! I actually have had my eye on the issue of require('pg') returning a weird singleton pool thing for a long time so last week I started breaking the pool out into its own module: pg-pool. I have a branch of node-postgres which removes all the pooling code from node-postgres directly & instead leans on this pooling module here. So far the changes are backwards compatible I think, which is neat. Anyways...this is gonna cause some epic levels of rebase problems for this PR unfortunately. If you'd like I can integrate your changes into pg-pool. If I do so mind if I ping you for a review over there?
@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Jun 22, 2016

Contributor

If I do so mind if I ping you for a review over there?

Sounds good to me. FWIW I am about to leave my company, and (probably) server side Javascript, but I'm happy to look at a patch if you send one!

We've been running this in production for about three weeks now without any problems.
Contributor

kevinburke commented Jun 22, 2016

If I do so mind if I ping you for a review over there?

Sounds good to me. FWIW I am about to leave my company, and (probably) server side Javascript, but I'm happy to look at a patch if you send one!

We've been running this in production for about three weeks now without any problems.
@abenhamdine

This comment has been minimized.

Show comment
Hide comment
@abenhamdine

abenhamdine Sep 1, 2016

Contributor

IMHO this feature would be very very useful.
This feature really miss because it's not so rare that you exhaust unintentionally the pool of connections, and currently if the case occurs pg says nothing about it.

For instance, I declared a pool of connections in my integration tests, in addition to the pool declared in the server, so the pool was not big enough.
It took me a long time to find out the reason... 😞

@kevinburke @brianc when can we expect this change to be available in node-pg-pool ?
Contributor

abenhamdine commented Sep 1, 2016 •
edited
Edited 1 time
  • @abenhamdine abenhamdine edited Sep 19, 2016 (most recent)

IMHO this feature would be very very useful.
This feature really miss because it's not so rare that you exhaust unintentionally the pool of connections, and currently if the case occurs pg says nothing about it.

For instance, I declared a pool of connections in my integration tests, in addition to the pool declared in the server, so the pool was not big enough.
It took me a long time to find out the reason... 😞

@kevinburke @brianc when can we expect this change to be available in node-pg-pool ?
@TooAngel

This comment has been minimized.

Show comment
Hide comment
@TooAngel

TooAngel Feb 2, 2017

I would really appreciate this option. Can I somehow help to get this merged?

TooAngel commented Feb 2, 2017

I would really appreciate this option. Can I somehow help to get this merged?
@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Feb 23, 2017

+1 for this

drmrbrewer commented Feb 23, 2017

+1 for this
@abenhamdine

This comment has been minimized.

Show comment
Hide comment
@abenhamdine

abenhamdine Feb 23, 2017

Contributor

@brianc @charmander could you pleeeeeaaase look at this ?
It would be so useful 😢
Contributor

abenhamdine commented Feb 23, 2017

@brianc @charmander could you pleeeeeaaase look at this ?
It would be so useful 😢
@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Feb 23, 2017

Contributor
Contributor

kevinburke commented Feb 23, 2017

@abenhamdine abenhamdine referenced this pull request Feb 23, 2017

Open

Suitable values for poolSize, poolIdleTimeout and reapIntervalMillis #1222

@abenhamdine

This comment has been minimized.

Show comment
Hide comment
@abenhamdine

abenhamdine Feb 23, 2017

Contributor

I think this has been implemented but in a different way - check out pg-pool

Could you elaborate ?
Are you talking about this => coopernurse/node-pool#127 ?
Because it seems to have been merged in node-pool v3, yet pg seems to be locked with "pg-pool": "1.*",
Contributor

abenhamdine commented Feb 23, 2017

I think this has been implemented but in a different way - check out pg-pool

Could you elaborate ?
Are you talking about this => coopernurse/node-pool#127 ?
Because it seems to have been merged in node-pool v3, yet pg seems to be locked with "pg-pool": "1.*",
@charmander

This comment has been minimized.

Show comment
Hide comment
@charmander

charmander Feb 23, 2017

Collaborator

pg-pool isn’t generic-pool, but pg-pool 1.x does still depend on generic-pool 2.x.
Collaborator

charmander commented Feb 23, 2017

pg-pool isn’t generic-pool, but pg-pool 1.x does still depend on generic-pool 2.x.
@abenhamdine

This comment has been minimized.

Show comment
Hide comment
@abenhamdine

abenhamdine Feb 23, 2017

Contributor

pg-pool isn’t generic-pool, but pg-pool 1.x does still depend on generic-pool 2.x.

Oups yes sorry you're right. However main point is that generic-pool 3.x is not used yet by pg.
Contributor

abenhamdine commented Feb 23, 2017

pg-pool isn’t generic-pool, but pg-pool 1.x does still depend on generic-pool 2.x.

Oups yes sorry you're right. However main point is that generic-pool 3.x is not used yet by pg.

@abenhamdine abenhamdine referenced this pull request Feb 24, 2017

Closed

Precision about connexion release #286

@brianc brianc added this to the pg@7.0 milestone May 24, 2017

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc May 24, 2017

Owner

I think having a connection timeout is really important for production systems. Sorry for the delay in getting to this. I recently (as of last week) quit full-time work so I could focus a lot more time on this library. I added this to the 7.0 milestone which I intend to start working on at the beginning of next week! 💃
Owner

brianc commented May 24, 2017

I think having a connection timeout is really important for production systems. Sorry for the delay in getting to this. I recently (as of last week) quit full-time work so I could focus a lot more time on this library. I added this to the 7.0 milestone which I intend to start working on at the beginning of next week! 💃
@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jul 14, 2017

Owner

I've added this in pg-pool@2.0 which works with node-postgres 5.x & greater. Sorry for the delay...but happy day! 💃
Owner

brianc commented Jul 14, 2017

I've added this in pg-pool@2.0 which works with node-postgres 5.x & greater. Sorry for the delay...but happy day! 💃

@brianc brianc closed this Jul 14, 2017

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