Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto reconnection to the database server is not triggered #257

Open
bleugateau opened this issue May 10, 2021 · 8 comments
Open

Auto reconnection to the database server is not triggered #257

bleugateau opened this issue May 10, 2021 · 8 comments

Comments

@bleugateau
Copy link

bleugateau commented May 10, 2021

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
Copy link

marcelc63 commented May 15, 2021

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:
Screen Shot 2021-05-15 at 17 02 40

I will try to investigate further as well.

Thanks!

@bleugateau
Copy link
Author

bleugateau commented May 17, 2021

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/[email protected]/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.

Log:
auth_1 | Mon May 17 2021 11:28:57 GMT+0000 (Coordinated Universal Time) - [CRITICAL]: Error: Broken pipe (os error 32)
auth_1 | at SendPacket.send (https://deno.land/x/[email protected]/src/packets/packet.ts:34:13)
auth_1 | at async PoolConnection.execute (https://deno.land/x/[email protected]/src/connection.ts:262:7)
auth_1 | at async PoolConnection.query (https://deno.land/x/[email protected]/src/connection.ts:239:20)
auth_1 | at async https://deno.land/x/[email protected]/src/client.ts:86:14
auth_1 | at async Client.useConnection (https://deno.land/x/[email protected]/src/client.ts:107:14)
auth_1 | at async Client.query (https://deno.land/x/[email protected]/src/client.ts:85:12)
auth_1 | at async MySQLConnector.query (https://deno.land/x/[email protected]/lib/connectors/mysql-connector.ts:79:22)
auth_1 | at async Database.query (https://deno.land/x/[email protected]/lib/database.ts:240:21)
auth_1 | at async Function._runQuery (https://deno.land/x/[email protected]/lib/model.ts:228:21)
auth_1 | at async Function.first (https://deno.land/x/[email protected]/lib/model.ts:550:21)

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

Best regards

@bleugateau
Copy link
Author

bleugateau commented May 21, 2021

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

  1. Add some code for after an "Error" try to reconnect the client in 1 attempt. (by argument in Connector.query())
  2. 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/[email protected]/src/packets/packet.ts:98:21)
at async ReceivePacket.parse (https://deno.land/x/[email protected]/src/packets/packet.ts:48:17)
at async PoolConnection.nextPacket (https://deno.land/x/[email protected]/src/connection.ts:164:16)
at async PoolConnection.execute (https://deno.land/x/[email protected]/src/connection.ts:263:21)
at async PoolConnection.query (https://deno.land/x/[email protected]/src/connection.ts:239:20)
at async https://deno.land/x/[email protected]/src/client.ts:86:14
at async Client.useConnection (https://deno.land/x/[email protected]/src/client.ts:107:14)
at async Client.query (https://deno.land/x/[email protected]/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
Copy link
Owner

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
Copy link
Author

bleugateau commented May 24, 2021

  • 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
Copy link
Owner

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
Copy link
Author

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
Copy link

eikmei commented Apr 25, 2023

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

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants