Node-postgres: "Called end on pool more than once"

[rightaway]

I'm getting an error "Called end on pool more than once" when I run close() on the pool when shutting down the application. I am doing it from the unhandledRejection handler to cleanly shut down in case of an unhandled rejection. But since this can be called more than once, close() is getting called more than once.

Is there any reason why this should cause an error? Can't it just silently fail if called more than once? Isn't calling it from a callback like unhandledRejection, which can get called many times, a standard use case?

[charmander]

Is there any reason why this should cause an error?

Because it’s a bit weird and it’s unclear what to do with the callback, assuming you mean .end() instead of .close(). “A bit weird” is enough to ask for the user to do something explicit, e.g.

let endedPool = false;

process.on('unhandledRejection', error => {
    ⋮
    if (!endedPool) {
        endedPool = true;
        pool.end();
    }
    ⋮
});

[rightaway]

Yes I mean end(). Maybe I'm not understanding what you meant, but if this endedPool check were handled by the pool itself then the callback could simply be this which is a lot clearer. What would be lost by moving this check into the pool? Since subsequent calls to pool.end() should always be a no op then wouldn't it be better to let the pool take care of all this?

process.on('unhandledRejection', error => {
        pool.end();
});

[charmander]

You can also do this:

pool.end(() => {});

[rightaway]

I notice that when pool.end is called without a callback a Promise is returned, but nothing is returned when called with a callback.

I'd like to do something like this in an error handler shutdown method. Wouldn't I need a Promise returned by pool.end(() => {}) for it to work like that, otherwise the process could be terminated before pool.end finishes successfully?

Promise.all([ pool.end(() => {}), something1.end(), something2.end() ]).then(() => {
    console.log("Closed everything safely.");
    process.exit()
});

[joelmukuthu]

@rightaway, yes you'd need a Promise returned for that. @charmander's point was that with pool.end(() => {}), you don't see any errors from pool.end. If an error occurs, for example if pool.end is called more than once, the method will call your fake callback (() => {}) with the error and then nothing happens.

If you do want the promise, then do pool.end(). But then be aware that you might want to handle errors if any of those things fails.

[rightaway]

I see, then I guess my earlier question remains:

Maybe I'm not understanding what you meant, but if this endedPool check were handled by the pool itself then the callback could simply be this which is a lot clearer. What would be lost by moving this check into the pool? Since subsequent calls to pool.end() should always be a no op then wouldn't it be better to let the pool take care of all this?

[joelmukuthu]

Well, I wouldn’t consider it a feature that pool.end can be called more than once. If we assume pool.end to be closing connections, it doesn’t make sense that it can be called more than once, since that’s not how connections work in general. IMO it’s doing exactly the right thing, throwing an error if it’s called multiple times, and that the inverse would be a bug.

If there are cases that cause it to be called more than once on your application, then it makes more sense to handle that there. If anything, your code will serve as documentation for why pool.end is called multiple times.