Fix for issue #964: connection error prevents pool from ending connection #964

[manuguerra]

When an error event bubbled up to PoolConnection, pool._removeFromPool was being called, so the connection was being removed from the array but not destroyed. Then, when pool.end was called, the connection wasn't being ended, since we didn't have any reference to it anymore.

This pull request adds a test to cover that issue and fixes it by removing the call to pool._removeFromPool on connection error, since it's not necessary.

[dougwilson]

So, adding a function that swallows all errors is not the correct fix; errors need to be handled. The references issue needs to be fixed, yes, but simply swallowing all errors is not the correct way.

[manuguerra]

@dougwilson, I agree with you, but is _removeFromPool really necessary here, since it's also being called on end events?

[dougwilson]

Is there a guarantee that all errors will be followed by an end event? Also, how does this fix #964 ? The end event is still not being fired on the query object.

[dougwilson]

Also, how does this fix #964 ? The end event is still not being fired on the query object.

Nevermind, I read the test incorrectly.

[dougwilson]

but is _removeFromPool really necessary here, since it's also being called on end events?

Is there a guarantee you can point to me in the code that end will always be fired after error for where you change? Even then, I still won't add this in; adding an empty function as an event listener is a huge code smell. It tells me there is a bigger problem somewhere and this fixes a symptom of it. We need to fix the actual issue and not add an empty event listener, basically.

[dougwilson]

Basically, it sounds like the bug is in the interaction between _removeFromPool and pool.end.

[dougwilson]

If it helps, I looked into this a while ago, and what I found needed to be fixed was https://github.com/felixge/node-mysql/blob/048f2c1040b760f0c15fe800689b590343df0f1c/lib/protocol/Protocol.js#L366-L368 was not correctly getting invoked, leaving the pool.end hanging.

[dougwilson]

And I believe it was because the error delegation calculation was wrong for the specific case.

[dougwilson]

Also, the on('error', fn) in the PoolConnection may need to call destroy on itself, as for what the error listener should probably be doing.

[dougwilson]

P.S., I'm not saying your solution here doesn't solve the problem or not, but the change just smells wrong to me, so I'm very iffy in accepting it without knowing more. Ideally there are not empty event listeners, but if there is absolutely no other way to fix it without breaking backwards-compatibility, I'm fine, but essentially I would accept changing it to an empty event listener as a last resort if there is no other possible solution.

[manuguerra]

I totally agree with you @dougwilson, and thank you for the clarifications.
I will dig deeper into this issue and will let you know if I have other ideas or questions.

[dougwilson]

No problem :) I look forward to this being fixed, and it's really awesome that you included a test! I don't mean to sound harsh or anything :)

[manuguerra]

Sure, thanks! :)