Make sure that psql socket disconnection is handled properly.

[DenizPiri]

No description provided.

[brianc]

I appreciate the pull req. What's the motivation? What problem are you solving?

[DenizPiri]

Hi,

It solves 2 problems:
1- Handling disconnected postgresql connections and raising the 'end' event as an end result.
You can test it by killing the postgtes processes while the client is connected or by stop'ing the server.
2- After the socket disconnection, cancelling all the queued queries and automatically calling the callback with an error for the queries trying to be made later on. Since this one changes the behavior, I am not sure if this should be included, it changes the behavior after all. But, if not that, then it would be nice to add an optional timeout parameter to queries,, which would apply to all queries, including the ones in the queue. This way, async flow would never halt because of postgresql never doing the callback call to the query.

I am not a javascript programmer, so feel free to change the patch as you wish. If you like I can also modify it as you wish.

[DenizPiri]

For the problem 2. An alternative solution could be to provide a function to kill of all the queued queries, by properly calling their callback functions with an error.
Imagine the following scenario: You start a tcp server, start accepting connections, lets say that for each connection you do a postgresql query, and as a result of that query, you send some data, and end the connection. Then, you want to gracefully shutdown this app while you are still getting lots of connections and you have quite a bit of queries in the queue. You send an SIGTERM to your app, you handle that within your app, and you call pg_client.end(),you clear your timeouts, intervals, etc.. But, when you do pg_client.end(), only active query gets cancelled currently, and queued queries just stays there. Causing all the socket connections that were waiting to be closed as a result of the query to stay active. Resulting in the application to never go down.
Another alternative could be to queue end() to the same queue that queries are queued into. And prevent immediately fail any further queries after end() gets called.

[brianc]

I think I see what you're saying now. To rephrase as verification we're on the same page:

When a client is disconnected from the backend PostgreSQL server if it is currently handling a query that query's callback will be called with the error; however, all queued queries within the postgres client will never fire their callbacks, resulting in scenarios where other listeners or sockets are waiting on the callbacks of queries which will never fire.

If that's the case, I definitely think fixing that is the way to go. As far as performing graceful shutdowns is concerned there are two different scenarios I can think of.

1. Using the built in pooling. Call pg.end() in your shtudown routine. This will prevent the pool from handing out new clients, but the pool will wait until all "busy" clients are returned before killing them. Shouldn't be an issue there with queries which never call their callbacks

2. Using custom pooling or some global clients to do the work. In this case since you'e opted to implement your own pooling you're also responsible for implementing the shutdown behavior I would suppose. That being said, I don't think the "what happens when you call client.end() on a client with queued queries" has actually been determined. There is some behavior currently but not sure it's been tested for or well understood. Sooo...I think looking at this question might help give a more deterministic solution to "what happens when I want to close down a client that has a large back-log of unfinished queries."

Thinking about point number 2 a bit more I would actually say the way to do this gracefully is as follows:

var backloggedClient = new pg.Client('whatever');
backloggedClient.connect();
for(var i = 0; i < 1000; i ++) {
  backloggedClient.query('SELECT NOW()');
}
//so now we have 100 pending queries but we want to end the client?
//end the client when it's queue drains
backloggedClient.on('drain', backloggedClient.end.bind(backloggedClient));

As far as an optional timeout parameter...I think this could be implemented in user-land code without too much trouble.

[brianc]

I appreciate the pull request. Error handling is super tricky & the more the better. Is there any way you could get these tests passing?