Auto reconnection to the database server is not triggered

bleugateau commented 3 years ago

Hello,

I discovered a issue in MySQL connection when it is auto disconnected by the server (connection timeout, ect), the connection is not reconnected on the first request to server.

Auto reconnection is trigger after 2-3 requests to server.

If I have time to investigate I post some update in this ticket.

Best regards.

marcelc63 commented 3 years ago

Hi adding data points to this, happened to me as well.

I confirm the behavior that reconnection does not happen on the first request and requires 2-3 requests to the server.

Following is my log:

I will try to investigate further as well.

Thanks!

bleugateau commented 3 years ago

I think we need to detect in "denodb" when connection is disconnected (ConnectionState ?), and reconnect it.

I have do small fix in my local denodb version, but that "ugly" trick for reconnect the connection in case of error. The trick is: try catch at MySQLConnector.query (https://deno.land/x/denodb@v1.0.38/lib/connectors/mysql-connector.ts:79:22), if an error has occurred force close and reconnect the connection and re-run queries that have not already been executed correctly.

@eveningkid: Hi have you any idea in your side ?

Best regards

bleugateau commented 3 years ago

Update: I have found two ways for fix the problem that not clean at all but that do auto reconnection and execute the query:

Add some code for after an "Error" try to reconnect the client in 1 attempt. (by argument in Connector.query())
Close client after each query, for force to makeConnection() (by argument in Connector constructor)

But I think that just "ugly bypass" for avoid "Broken Pipe", "Connection timed out", "Connection Abort".

What do you think about this issue ? @eveningkid I can do two pulls requests for show you the two ways.

Log:

INFO connecting 127.0.0.1:3308 INFO connected to 127.0.0.1:3308 DenoDB testing. STATEMENT tentative INFO close connection Catched error ! ConnectionAborted: (os error 10053) at unwrapOpResult (deno:core/core.js:100:13) at async read (deno:runtime/js/12_io.js:97:19) at async ReceivePacket.read (https://deno.land/x/mysql@v2.8.0/src/packets/packet.ts:98:21) at async ReceivePacket.parse (https://deno.land/x/mysql@v2.8.0/src/packets/packet.ts:48:17) at async PoolConnection.nextPacket (https://deno.land/x/mysql@v2.8.0/src/connection.ts:164:16) at async PoolConnection.execute (https://deno.land/x/mysql@v2.8.0/src/connection.ts:263:21) at async PoolConnection.query (https://deno.land/x/mysql@v2.8.0/src/connection.ts:239:20) at async https://deno.land/x/mysql@v2.8.0/src/client.ts:86:14 at async Client.useConnection (https://deno.land/x/mysql@v2.8.0/src/client.ts:107:14) at async Client.query (https://deno.land/x/mysql@v2.8.0/src/client.ts:85:12) reconnectionAttempt true INFO connecting 127.0.0.1:3308 INFO connected to 127.0.0.1:3308 STATEMENT answer >> Flight { destination: "Tokyo" }

eveningkid commented 3 years ago

Hey,

Thank you so much for looking into this issue, really.

Here's a few notes:

The MySQL driver used by denodb does not provide any way to see if the connection has timed out. So this has to be checked on our end indeed
There is a pool timeout option in the driver configuration. Maybe this could be added to our MySQL connector?
If we have a reconnection mechanism, it has to be there by default and not impact performance if possible. I think your second suggestion as you said might be overkill

Now I wonder: do you think we could only catch this type of error? Is there maybe an error code we can look for and only then call for a reconnection?

By the way, the simplest way for a new reconnect() method is to set the connector to disconnected and call makeConnection() again (but I'm sure you got that already!).

Let me know if you think we can specifically target this type of error inside our try/catch! :)

Thanks again for helping on this! I'll try to be more reactive next time (you know how life is)

bleugateau commented 3 years ago

Previously I looked for something like an event dispatcher when an error happened in" mysql driver" source, but there is nothing about it. Maybe we can make pull request in the driver for emit() when an error is provided or when connectionState has changed. And call our reconnect() function.
We can usethe idleTimeout param to set it to 1 min for do something like "heart break packet", that do auto re-connection if I understood the "mysql driver" source correctly. (but that just an another trick)
Yes totally

Yes they have a lot of typed classes for determine errors: https://github.com/denodrivers/mysql/blob/6dcab807b9794376d818660761e2190524d3dcb0/src/constant/errors.ts And the error parser provide "code" property: https://github.com/denodrivers/mysql/blob/6dcab807b9794376d818660761e2190524d3dcb0/src/packets/parsers/err.ts

I need to investigate more, this week I don't have much time to do it, I have a lot of work. But when I got free time, I watch out

Have a good day, and no problem dude ! It's a pleasure for me

PS: Sorry for my bad english, it's not my primary lang

eveningkid commented 3 years ago

I think we can indeed handle it on our side, it's not necessary to PR the MySQL driver.

We could just go with the try/catch approach you mentioned, then look for a ResponseTimeoutError I suppose.

Then if it is a timeout error, call reconnect():

reconnect() {
  this._connected = false;
  return this._makeConnection();
}

I guess we could have something like this for the query section:

try {
  ...
} catch (error) {
  if (error instanceof ResponseTimeoutError) {
    await this.reconnect();
    return this.query("pass the arguments from the current query call [I forgot the code signature /o/]");
  }
}

And no worries for the language, je suis français aussi :)

bleugateau commented 3 years ago

This is what I have been doing since the beginning, I didn't think it was the cleanest way to do it 😄

I make PR ASAP !

Bonne journée

eikmei commented 1 year ago

Hello, I'm currently facing the same issue while syncing from MySQL. May I know if the fix (https://github.com/eveningkid/denodb/pull/269) will be applied soon? 😁 And my current Deno version is v1.18.0, will the version affect too?

eveningkid / denodb

Auto reconnection to the database server is not triggered #257