Fails to reconnect after invoking end()
vitaly-t opened this issue · 12 comments
We call end()
on the pool in order to shut down all the connections within the pool.
However, doing so prevents us from being able to use the object any longer.
Any attempt of calling .connect
after .end()
was called once results in the following error:
pool is draining and cannot accept work
Below is a complete test application:
'use strict';
var pg = require('pg');
var config = {
database: 'newone',
port: 5433,
user: 'postgres'
};
var pool = new pg.Pool(config);
function test1() {
pool.connect((err, client, done) => {
if (err) {
console.log('Error Connecting:', err);
return;
}
client.query('select count(*) from users', (err, data) => {
if (err) {
console.log('Query Error:', err);
} else {
console.log('DATA:', data.rows);
}
done();
pool.end();
test2();
});
});
}
function test2() {
pool.connect((err, client, done) => {
if (err) {
console.log('Error Connecting:', err);
return;
}
client.query('select count(*) from users', (err, data) => {
if (err) {
console.log('Query Error:', err);
} else {
console.log('DATA:', data.rows);
}
done();
pool.end();
});
});
}
test1();
test1
ends successfully, but in test2
it always throws that error on this line:
console.log('Error Connecting:', err);
//=> pool is draining and cannot accept work
Yeah - once you call end()
on the pool it's over: it's being disposed of. You'll need to create a new pool. That's behavior as designed.
But why? What is the reason for that?
That's how generic pool works: https://github.com/coopernurse/node-pool#draining
I don't think it matters how that pool works though.
You encapsulate that pool in your library, which means you can set it to null
within the end
function, and then create a new one within the connect
function, if required.
This would make it consistent with how connections used to work, and still work via pg.connect
, that we can call .end()
and then call connect
after that successfully.
P.S. It mostly inconveniences automated tests, but sometimes also real applications, if you do not have direct access to the object that holds the pool, and want to execute queries indirectly.
or....maybe it is for the better...? I'm still debating over the cons and pros of it...
@brianc If we agree that this is ok for the pool to shut down itself permanently, then we definitely need a way to determine when this has happened.
When we share a Pool
object we need to be able in some situations to ask - Was this pool terminated / ended / finished / capoot
?
Something like this:
if(pool.isTerminated) {
// do what needs to be done
}
When using the pool inside another high-level library it requires some intelligence, in order to provide good diagnostics, that's what I'm working on now.
Why would you destroy the pool if you still wanted to use it?
@charmander as per an earlier example, a Pool
object is exposed by the library, to be shared with other third-party libraries or modules, in order to avoid creation of an additional pool.
A third-party library / module may incorrectly end the pool, thus breaking all the other modules that use it.
As I wrote earlier, it is probably wise to keep it she Pool
object destroyed, but we at least need a way to determine that, so it can be diagnosed easily, something like pool.isTerminated
, see the example above.
but we at least need a way to determine that, so it can be diagnosed easily
Maybe the message saying the pool is draining? That’s what .end()
does. You could submit a wording pull request on generic-pool.
Maybe the message saying the pool is draining
We don't need any message, just an indication/flag, so any library can provide its own message.
You could submit a wording pull request on generic-pool.
Addition of such a flag would be trivial. I will do it now 😉
I've just found out that we can verify it through the generic-pool
library:
var p = new pg.Pool(connection);
var isTerminated = p.pool._draining;
though I'm not sure if it is a very good one, to rely on an internal module for this. It can change in that module, that in turn will break our module that uses node-postgres
. Probably best is still to add that isTerminated
property.