The connection parses errors out and emits them just as any other message so they can be handled higher up. Either by the client if a query is executing or emitted into the client's application. Not emitting errors basically removes any indication that anything has gone wrong including a query failure, bad query syntax, a disconnection...anything. Swallowing errors is a total no-go, sorry. What is the actual problem you're running into?

@brianc The problem I'm running into is #746. I also wrote up a short analysis of what's going wrong in there.

Swallowing errors is definitely not the intention of the patch, although I do realize now that that is what's happening. However, the duplicate error and the inability to handle that error is definitely a problem. I'd be happy to work on a proper solution once I understand all aspects of the error handling.

Ah I see now - much more context. Yeah so eating the error is probably not the right approach here. In the case of a bad password some installs of postgres will disconnect the socket after a bad password attempt. This causes the socket to emit an error, which is passed up via the connection -> client like you were describing. Problem is, which you also outlined, is that the client already received an error message from postgres about the bad password. The right way to handle this is probably something like this...

if the client is trying to connect and the stream emits an error or the client receives an error message and the client is pooled...then suppress any further error messages on the client within the pool, and dispose of the client.

I haven't been able to fix this because I don't have access to an instance of postgres that has the double-error problem, but if you wanna take a crack at fixing it there in the pool I'd 100% ❤️ to merge the PR!

Awesome. I'll try to find a proper solution.

I'm really wondering about this double-error thing though. I was recently looking into a very similar issue at voxpelli/node-pg-pubsub#6 and I'm assuming @voxpelli never experienced this either.

I think I saw mentioning that this might be a Windows issue, which seems reasonable, given that I'm on Windows.

I hope I'm not causing too much trouble with the pushes, but I can't run the tests on Windows locally so far, so I'm letting travis handle that.

No trouble at all. I only get travis email on master pass/fail or PRs I am
the submitter of. Have at it!

On Mon, Aug 24, 2015 at 10:09 AM, Oliver Salzburg notifications@github.com
wrote:

I hope I'm not causing too much trouble with the pushes, but I can't run
the tests on Windows locally so far, so I'm letting travis handle that.

—
Reply to this email directly or view it on GitHub
#843 (comment).

@brianc I'm a bit stuck right now. The test failure is telling me that client.on is not a function. But EventEmitter.call(this); is the first thing happening in the Client constructor. Any idea what's going on?

Okay, so it's a FakeClient during testing. But that inherits from EventEmitter as well. I'm still confused :P

Okay, got it, I was missing the mocked Client type. I opted to add a noop on to it, The only new thing that I added that uses on, only uses it to register a noop event handler. So that seemed pretty safe.

@brianc You mentioned disposing the client. From what I can tell, that should already be happening, because all the connection-related error handling is already in place. With the current approach, only the one specific case where two (or multiple) errors are generated, before a connection has been established, is handled. Is that enough?

@brianc If you could provide some feedback on this, I'd really appreciate it :)

@oliversalzburg sorry for taking a while to get to this - I think this actually looks really good! Also - that's a cool trick for noop using function.prototype - I didn't know that one! I'll merge this & push a new patch version right away. :)

Awesome! 👍 :)