Catch errors client throws in pool

[zlotnika]

Old behavior: if you do something like

try {
  await (new Pool()).query(null);
} catch (error) {
  console.log(error);
}

you get an uncaught exception. If you have a server that generates text to pass in to query the pool, it will crash your server.

New behavior: This change catches the error thrown at https://github.com/zlotnika/node-postgres/blob/master/packages/pg/lib/client.js#L505 and treats it the same as it does with any other. This way, a query of null works similarly to a query of 'over 9000' or new Date() or whatever other not-so-SQL-y thing you come up with.

An alternate solution would be to call the callback with an error in the client instead of throwing an error. I personally prefer throw-catching.

[bryanph]

I found the same is true for when failing to connect to the server, there's no way to catch the error. It simply crashes the app. I suppose this would resolve that as well. Would be great to see this merged 😄

[brianc]

I apprecaite the contribution here. You're right, throwing errors on a method that accepts a callback or returns a promise is not a nice pattern. I will be happy to merge this if you write test coverage for the behavior. No tests, no merge for the most part as w/o tests its impossible for me to maintain backwards compatibility and prevent regressions long term. If you don't have time to write a test, I understand. In that case I'll try to come back & write test(s) for this & get it merged as soon as I can!

[benk79]

IMHO this is a crucial point for an application to run in production. Error management is really a must, not just a nice to have. Tryed to see if i can write a test, but had so many already existing tests failing that gave up.

[benk79]

My exact problem being almost the same as @bryanph but also for DB server being shut down or having failures, i finally ended with this simple solution

pool.on('connect', client => {
    client.on('error', (err)=> {
        pool.emit('error', err, client);
    });
});

Then handle the error from pool.on('error', fn);

[zlotnika]

I've tried writing a test, but I can't seem to get testing working on my machine. It looks like there's a lot of stuff set up for this, but no clear documentation. Could anyone either 1. write a test for me or 2. add something on running tests to the Readme or 3. tell me where else to find info on it?

[zlotnika]

Alright. I've added a test that should work right. My environment is a bit different, so hopefully it'll run correctly on travis etc. Let me know if it works or whatever. Feel free to suggest fixes for it!

[josh-cain]

@brianc Is this PR still being considered? Have a problem similar to #2439 and was hoping to get something here in the upstream rather than fix elsewhere.

[bbeesley]

Hey @brianc is there any chance of some feedback on this PR? We've been suffering this issue using node-postgres with big aws aurora clusters where I work. We keep getting paged about it since we're getting containers dying in production every 15 mins or so. Is there any reason not to merge this PR? If you need better test coverage then I'm happy to help @zlotnika out there. Just hoping to be able to avoid having to fork this repo and publish a patched version of it since it's overhead I'd much rather do without.

[brianc]

yes i am sorry - you're absolutely right this should be merged, and I will do that now & get a new release out tomorrow. There's some epic storm in Austin right now so...my power is going in and out but i'll get ya patched up tomorrow along w/ a salvo of other fixes. Sorry for the delay!