Nice. But it makes me wonder why it is needed.
You would expect, node wise, that if the callback has both an error and 'result' parameter, and if the error is filled, the client is invalid. That is also expected looking at the flow: make a client, if err: some error ocurred making a client, thus the client is invalid.
As done() is to return a client to the pool, why should this be called on an invalid client?

I think a better solution is to make node-postgres prevent the insertion of such invalid, errored client, to the pool, making the call to done() not needed.

I did not look at the source writing this, so it might not be possible in the design.

You have a point there. I have not looked at the code either, which I should have actually done to start with. In any case, I found myself running out of clients due to the done() not being called on errors and this was a quick and easy fix.

Ooh quick 'n easy!

I'll look into it. But the done requirement for error'd connections sounds like a nogo for me.
There are many problems on the pool, also because it is not easy to test different locations of connection errors.

On Sep 1, 2014, at 10:05 AM, Stefan Aichholzer notifications@github.com wrote:

You have a point there. I have not looked at the code either, which I should have actually done to start with. In any case, I found myself running out of clients due to the done() not being called on errors and this was a quick and easy fix.

—
Reply to this email directly or view it on GitHub.

So I see this line: https://github.com/brianc/node-postgres/blob/master/lib/pool.js#L75
If the error you try to fix, walks this path: so the pool.acquire has an error, then the client passed to the user is null, and the done function doesn't do shit. So calling it won't matter.

Now, if it does indeed leak if you don't call done(), it means it does not go there.

A couple of lines lower, you see the definition of the done() function: you should pass the error, so that the client is destroyed instead of released. I think this error is for connections that will fail to do anything else. If a simple query failed, the connection is still usable. So you should call done() if a connection error occurs.

You can also see that it calls the callback you supplied there with error as null. So either you have an error in connect(), and it gives you an error and a NOOP done(), or you have an actual client and a usable done(). However, then error is null.

pg.connect(function(err,client,done) {
   // Either err contains an error, then err = Object, client = null, done = NOOP
   if(err) {
       // done(); // useless
       throw 'Oh noes!'; // Fail, do not do any other stuff
   }
   // Or: err = null, client = Object, done = Function
   // ....
   client.on('error',function(e) {
      console.log('Oh Noes!');
      done(e); // I think this should solve it
   });
   done(); // releases client
});

Is there anywhere in your code where you catch errors of the client (connection errors, client.on()), and call done(), but with no error supplied? If that is the case, you would, I think, run out of clients because no usable clients are in the pool.

Well, I have to say, that makes much more sense. I went to look at the code and you are right. If calling the done() on the error was making any actual difference, then that should be tested. When I have some more time I will get back into it.

Thank you so far.