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

Support timing out acquire() call #127

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@kevinburkeshyp
Copy link
Contributor

kevinburkeshyp commented Feb 17, 2016

The options dictionary for pool.acquire() now takes a second argument,
timeout. Pass an integer number of milliseconds to cancel the acquire() call
and hit the callback with pool.Full after the given amount of time has elapsed.
Hits the callback with an immediate error if the timeout is a negative number,
zero, or NaN.

Adds a number of tests that this logic behaves as expected.

This work was sponsored by Shyp.

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Feb 17, 2016

Note, this builds on #125, so both commits are present in the diff. Happy to rebase/merge/submit against the branch/whatever you like.

kevinburkeshyp pushed a commit to Shyp/node-postgres that referenced this pull request 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.

@kevinburkeshyp kevinburkeshyp force-pushed the Shyp:timeout branch 2 times, most recently from a0efc05 to 531a699 Feb 18, 2016

@sandfox sandfox added this to the 2.5.0 milestone Feb 20, 2016

@kevinburkeshyp kevinburkeshyp force-pushed the Shyp:timeout branch from 531a699 to 2eb4351 Feb 20, 2016

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Feb 20, 2016

I pushed 81c28ad with a fudge for the timer tests. I'm not too happy with it. I can rebase down before merge.

return false
}
} else {
if (typeof options.timeout === "number") {

This comment has been minimized.

@subeeshcbabu

subeeshcbabu Feb 24, 2016

Is it better to name this option as acquireTimeout to avoid confusion, if any. We have other timeouts like idleTimeout etc.

@subeeshcbabu

This comment has been minimized.

Copy link

subeeshcbabu commented Feb 24, 2016

+1

1 similar comment
@pdufour

This comment has been minimized.

Copy link

pdufour commented Mar 8, 2016

+1

@sandfox

This comment has been minimized.

Copy link
Collaborator

sandfox commented Mar 14, 2016

Just an update. I'm sorry for the delay, work and life have been rather hectic at the moment, but I've scheduled in some time to look at this after Wednesday. Thanks for the patience

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Mar 17, 2016

Removed the block option in 5e4bf20. I can rebase down before merge.

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Mar 17, 2016

Pushed the error callback to the next tick in 295e00a.

Kevin Burke added some commits Feb 16, 2016

Kevin Burke
Add option to error if the pool is full
Currently, if you attempt to acquire a resource and the pool is full, the
callback will not run until a space becomes available. This can lead to a
deadlock if a pool member's release is blocked on the acquire callback running.

To mitigate the deadlock possibility, support a third argument
`pool.acquire(cb, priority, {block: false})`, which will immediately hit the
callback with an error if the pool is full. Adds a test that this behaves as
expected.

Adds a helper function pool._isFull, which checks both the _count and the list
of _availableObjects - I think we have to check both to ensure that the pool is
full, and that this abstraction doesn't exist yet.

Callers may want to wait for a small amount of time before giving up on the
pool; I'm going to add that option in a second commit.

This work was sponsored by [Shyp](https://shyp.com).
Kevin Burke
Support timing out acquire() call
The `options` dictionary for pool.acquire() now takes a second argument,
`timeout`. Pass an integer number of milliseconds to cancel the acquire() call
and hit the callback with pool.Full after the given amount of time has elapsed.
Hits the callback with an immediate error if the timeout is a negative number,
zero, or NaN.

Adds a number of tests that this logic behaves as expected.

This work was sponsored by [Shyp](https://shyp.com).
Kevin Burke
Bump timeout to 20ms
Apparently the timer resolution isn't fine enough; I've observed this value as
high as 22ms, but I just ran the tests ~100 times with the diff set to 20ms.

If it's a problem I can bump higher or we can try to figure out some way to
force the ordering here with fake timers.
Kevin Burke
Remove block option
Replace block: false with timeout: 0, update code and tests. I will rebase this
down before it's ready for merge.

@kevinburkeshyp kevinburkeshyp force-pushed the Shyp:timeout branch from 295e00a to 07f3259 Mar 26, 2016

Kevin Burke
@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Mar 27, 2016

Lint errors should be fixed in 5556a81.

@sandfox

This comment has been minimized.

Copy link
Collaborator

sandfox commented Mar 30, 2016

thanks

(passive aggresive internal voice): I WILL finish looking at this soon and merge it!

@subeeshcbabu

This comment has been minimized.

Copy link

subeeshcbabu commented Mar 30, 2016

Great stuff. Waiting for this publish.

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Apr 11, 2016

@sandfox thoughts here? wondering if we should fork this lib & node-postgres or wait for upstream

@sandfox

This comment has been minimized.

Copy link
Collaborator

sandfox commented Apr 14, 2016

@kevinburkeshyp this is for reals actually open in my text editor and I'm looking at it now :-)

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented Apr 14, 2016

Thanks!

@kevinburke

This comment has been minimized.

Copy link

kevinburke commented May 2, 2016

I've published this PR as a new package here: https://www.npmjs.com/package/generic-pool-timeout and submitted it for inclusion as part of node-postgres here: brianc/node-postgres#1006.

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor

kevinburkeshyp commented May 27, 2016

hey @sandfox - anything I can do to help push this across the finish line? code review other PR's? make a donation?

kevinburkeshyp pushed a commit to Shyp/node-postgres that referenced this pull request 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.
@sandfox

This comment has been minimized.

Copy link
Collaborator

sandfox commented Jun 12, 2016

Hi Kevin,

Sorry for the haitus on this, I had a bit of collision of burnout, changing jobs, and some personal stuff.
Anywayaaay, onto code! Whilst looking at adding some other features I had to make some more big refactors on top you work and after various iterations and dead ends I've ended up realising alot of the future work isn't possible with jamming event-emitters into the lib/api.

The branch much features should have an implementation of the feature you are after, with some slight changes to accomodate other features. There are still a couple of test failures outstanding which I don't think can fixed without add event-emitters (which incidentally is needed to properly isolate the resource creation cycle/aspect from the resource issuance aspect/cycle, originally the two were too tightly coupled. A thing requesting a resource shouldn't care/know/etc about the resource factory having errors creating individual resources (but it will still obviously care about an overall failure to be able to create resources)).

The side effect of this is that the API change will probably mean a major version number bump which I wanted to avoid, but maybe that isn't a bad thing.

@sandfox

This comment has been minimized.

Copy link
Collaborator

sandfox commented Nov 23, 2016

closing as this is now included as part of v3

@sandfox sandfox closed this Nov 23, 2016

@abenhamdine abenhamdine referenced this pull request Feb 24, 2017

Open

merge node-pool v3 #1

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