Recognize fatal mysql2 connection errors

[novemberborn]

Connections created by MySQL2 driver transition to broken state when
network errors or timeouts occur. This condition is not visible to the
connection pool in Knex.

If Knex detects a timeout and tries to close a connection, this causes
an uncaught exception from MySQL2. Similarly the connection pool never
realizes the connection is in a broken state, and thus will never
replace it.

Unfortunately the MySQL2 connection does not expose a state, but it can
be inferred by checking for connection._fatalError.

Fixes #1798

---

This is an alternative to #1853. I believe the issue is specific to the mysql2dialect, however (see #1798 (comment) which shows the mysql2 client being used). The workaround proposed in #1853 likely fights only the symptoms, but does not solve the underlying problem.

Ideally https://github.com/sidorares/node-mysql2 would expose the connection state, but it's probably worth fixing the problem in Knex even for current releases of mysql2.

It's not clear to me if there are tests for this dialect. To be quite frank though I'm planning to switch my (business) client's project to the mysql dialect once mysqljs/mysql#1776 lands there, so unfortunately I can't justify spending a lot of effort on fixing issues with mysql2.

[sidorares]

Ideally https://github.com/sidorares/node-mysql2 would expose the connection state

do you know what's knex api for this? happy to add support on node-mysql2 side

[novemberborn]

@sidorares I think validateConnection corresponds to the validate call in generic-pool. See the documentation.

What's needed then is an official way to check whether a connection is still valid. This PR assumes that connections are valid unless they have a _fatalError. It also assumes that connections that do not have a _fatalError can be ended. If that's not the case then there should be a proper way of checking that before calling end().

[elhigu]

is that ._fatalError attribute part of public API of mysql2 driver?

[elhigu]

I suppose this is using private API of mysql2 driver, so it is very important to have some kind of test for this. Is ._fatalError just undefided or null before error happens?

[novemberborn]

I suppose this is using private API of mysql2 driver, so it is very important to have some kind of test for this. Is ._fatalError just undefided or null before error happens?

I think it's private, yes. It defaults to null: https://github.com/sidorares/node-mysql2/blob/b8f0b98e61e865736f4cc6917ce5fe70e6beff96/lib/connection.js#L84

[elhigu]

@sidorares is there or could some public API be added to mysql2 driver to be able to get this?

[sidorares]

@elhigu I'm happy to add anything that would work for you, ideally in a way compatible with mysqljs/mysql

what if I wrap https://github.com/sidorares/node-mysql2/blob/b8f0b98e61e865736f4cc6917ce5fe70e6beff96/lib/connection.js#L84 into a getter function and "bless" it to be public api?

[elhigu]

@sidorares sounds good 👍

[I00000]

👍

[elhigu]

@sidorares was that checking of fatal error brought to public API?

@novemberborn if not, then just a testcase for making sure that fatal error attribute exists (to prevent regression) and rebasing the branch would be enough to get this in.

[novemberborn]

This PR arose out of work I was doing for a client, and that contract has since ended (and they switched to Postgres). Unfortunately I won't have the time to pick this up from where I left off, so anybody reading this please feel free to take the commit and add a test case etc as necessary.

[elhigu]

Will be ropened if someone takes over finishing this.

[fluxsauce]

I understand that this isn't currently being worked on, but this is still an open bug IMHO. Hit the exact same problem, and our solution is going to be to switch to mysql, not mysql2

[elhigu]

@fluxsauce I couldn't tell from your comment if it was critique that this PR should not have been closed or just a clarification for people finding this issue that the bug still exist :)

Yes it is still open bug, related issue has not been closed. This PR was closed, because it is not been worked on, there is no benefit on leaving unfinished PRs open to bitrot.

[fluxsauce]

@elhigu ah, thanks for the clarification, I missed #1798 . Sorry about that!