Skip to content
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

connectionTimeoutMillis doesn't work #1390

Closed
bydga opened this issue Jul 31, 2017 · 6 comments
Closed

connectionTimeoutMillis doesn't work #1390

bydga opened this issue Jul 31, 2017 · 6 comments
Labels

Comments

@bydga
Copy link

bydga commented Jul 31, 2017

Im trying to use the connTimeoutMilis feature (respond error when a connection from a pool cannot be obtained within specified limit) - and it doesnt work.

Here is a minimal example:

{ Pool } = require 'pg'

process.on 'uncaughtException', (err) -> console.log "uncaught", err

pool = new Pool
	connectionString: "tcp://usr:pass@XXX.us-west-2.rds.amazonaws.com/db_name?ssl=true"
	max: 3
	connectionTimeoutMillis: 10000

pool.on 'error', (e, client) -> console.log 'pool.on.err', e

pool.connect (e, client, done) ->
	console.log "first", e
	pool.connect (e, client, done) ->
		console.log "second", e
		pool.connect (e, client, done) ->
			console.log "third", e
			pool.connect (e, client, done) ->
				console.log "fourth", e

output:

$ coffee pg.coffee
first undefined
second undefined
third undefined

and nothing more, no error thrown, no err callback being called..

versions:

$ npm ls | egrep "pg|postgr"
├─┬ pg@7.0.2
│ ├── pg-connection-string@0.1.3
│ ├── pg-pool@2.0.1
│ ├─┬ pg-types@1.12.0
│ │ ├── postgres-array@1.0.2
│ │ ├── postgres-bytea@1.0.0
│ │ ├── postgres-date@1.0.3
│ │ └── postgres-interval@1.1.1
│ ├─┬ pgpass@1.0.2
@brianc
Copy link
Owner

brianc commented Jul 31, 2017

✅ small self contained steps to reproduce
✅ listing of all the versions in use

Thanks for the nice bug report! I'll look at this ASAP.

@charmander
Copy link
Collaborator

@brianc, is this the intended behaviour of connectionTimeoutMillis? i.e. should it only apply when connecting, or should it apply when the pool has reached its limit and the caller has to wait for the next available client as well?

@ghafran
Copy link

ghafran commented Aug 10, 2017

There is a bug with 7.x where the pooled client never closes, no matter what the connectionTimeoutMillis is set to.

Even after calling done, the connection remains open indefinitely.
this.pool.connect(function (err, client, done) { done(); });

This works fine with 6.1.0. We have reverted back to 6.1.0 until this is resolved because its causing connection leaks.

@charmander
Copy link
Collaborator

@ghafran This issue is about connectionTimeoutMillis, not idleTimeoutMillis.

@brianc
Copy link
Owner

brianc commented Aug 10, 2017

@charmander thanks for drawing my attention back to this one. Got lost in the never-ending issue shuffle. I wanted connectionTimeoutMillis to apply both when initially connecting and waiting for a client from a full pool. The thinking behind that is you're calling connect either time regardless so it should always behave the same way. Likewise pool.query should reject after connectionTimeoutMillis if the timeout is exceeded. I'll look at this right away.

@charmander charmander added the bug label Aug 10, 2017
brianc added a commit to brianc/node-pg-pool that referenced this issue Aug 10, 2017
This addresses [pg#1390](brianc/node-postgres#1390).

Ensure connection timeout applies both for new connections and on an exhuasted pool.  I also made the library return an error when passing a function as the first param to `pool.query` - previosuly this threw a sync type error.
brianc added a commit to brianc/node-pg-pool that referenced this issue Aug 10, 2017
* Add connection & query timeout if all clients are checked out

This addresses [pg#1390](brianc/node-postgres#1390).

Ensure connection timeout applies both for new connections and on an exhuasted pool.  I also made the library return an error when passing a function as the first param to `pool.query` - previosuly this threw a sync type error.

* Add pg-cursor to dev deps
@brianc
Copy link
Owner

brianc commented Aug 10, 2017

@bydga - My apologies for the delay in responding here to the bug; I get a lot of issues and sometimes they slip off my radar...I've fixed it here. 👏 Please install the newest version of pg-pool and see if it fixes your issue!

Thanks

If you or your company benefit from node-postgres and have the means, please consider supporting my work on Patreon.

@brianc brianc closed this as completed Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants