fix: Auto reconnection to the database server is not triggered #269

bleugateau · 2021-06-03T19:10:00Z

Related to #257

…imeoutError happened

eveningkid

Alright so logic-wise it's all good!

All my comments are about keeping the codebase consistent/coherent (you couldn't have guessed it!).

It'll be ready to merge after these changes :)

Thanks again for helping and take your time, there's no rush

eveningkid · 2021-06-04T13:36:08Z

lib/connectors/mysql-connector.ts

@@ -4,6 +4,7 @@ import type { Connector, ConnectorOptions } from "./connector.ts";
 import { SQLTranslator } from "../translators/sql-translator.ts";
 import type { SupportedSQLDatabaseDialect } from "../translators/sql-translator.ts";
 import type { QueryDescription } from "../query-builder.ts";
+import { ResponseTimeoutError } from "https://deno.land/x/mysql@v2.8.0/src/constant/errors.ts";


This should be added to /deps.ts and then imported from there.

It's just to keep external dependencies cleaner, all in one place :)

MysqlClientResponseTimeoutError would be a good alias based on other imports from that driver

eveningkid · 2021-06-04T13:37:02Z

lib/connectors/mysql-connector.ts

@@ -46,6 +48,7 @@ export class MySQLConnector implements Connector {
      password: this._options.password,
      port: this._options.port ?? 3306,
      charset: this._options.charset ?? "utf8",
+      idleTimeout: this._options.idleTimeout ??  4 * 3600 * 1000


Just wondering why it's 4 hours by default? Do we need a default even?

eveningkid · 2021-06-04T13:42:15Z

lib/connectors/mysql-connector.ts

+  /**
+   * Executing query
+   * @param queryDescription {QueryDescription}
+   * @param client {MySQLClient | MySQLConnection}
+   * @param reconnectAttempt {boolean} - Used for reconnect client when ResponseTimeoutError happened
+   */
  async query(
    queryDescription: QueryDescription,
    client?: MySQLClient | MySQLConnection,
+    reconnectAttempt: boolean = true


I understand the intent but this connector follows an interface (Connector) and can't be tweaked for just this one.

Honestly, we could send this to the connector options instead but I don't see why someone wouldn't want it to reconnect and get an error instead you know what I mean?

So:

Let's keep query clean with just queryDescription and client?

Add reconnectOnTimeout?: boolean to MySQLOptions

Use this._options.reconnectOnTimeout instead of reconnectAttempt in this method

eveningkid · 2021-06-04T13:56:58Z

lib/connectors/mysql-connector.ts

+      let result = null;
+
+      try{
+        result = await queryClient[queryMethod](subqueries[i]);
+      }
+      catch(e){
+        //prevent unhandled behavior
+        if(!(e instanceof ResponseTimeoutError) || !reconnectAttempt){
+          return result;
+        }
+
+        //reconnect client, at this moment we can't subscribe to connectionState of mysql driver, we need to do this
+        await this.reconnect();
+
+        return await this.query(queryDescription, client, false);
+      }


This needs formatting (you can try deno fmt or install an IDE deno plugin that does it by default on file save)

I think it is more readable to go with the edge case in the if statement:

try { const result = await queryClient[queryMethod](subqueries[i]); if (i === subqueries.length - 1) { return result; } } catch (error) { if (this._options.reconnectOnTimeout && error instanceof ResponseTimeoutError) { await this.reconnect(); // No need for await below since there's no other statement after this return. // Also not passing `false` is fine, it can try many reconnections not just one return this.query(queryDescription, client); } // Other errors should still be raised throw error; }

No need for comments, the code is very readable (you did a good job already!)

eveningkid · 2021-06-04T13:58:20Z

lib/connectors/mysql-connector.ts

+  /**
+   * Reconnect client by close existing and reconnecting it
+   */


I think you can make it even simpler:

/** Reconnect current client connection. */

eveningkid · 2021-06-04T14:00:54Z

lib/connectors/mysql-connector.ts

+   * Reconnect client by close existing and reconnecting it
+   */
+  async reconnect(){
+    console.log('Debug >> Reconnect MySQL Client');


I know it isn't written anywhere but there's actually a helper for that :D
https://github.com/eveningkid/denodb/blob/master/lib/helpers/log.ts#L11-L13

Maybe we could use a warning for that and just say "Reconnecting MySQL client" yes

bleugateau · 2021-06-04T14:13:24Z

I will edit my pull request when I got free time, ahah I made a lot of small mistake.
My bad, I should have rechecked my fork before pushing it !

bleugateau · 2021-06-09T09:25:50Z

I'm so busy this moment sorry for the delay, this night I push new changes on PR.

bleugateau · 2021-06-16T21:58:01Z

Sorry for the delay, it's pushed!

eveningkid

Thanks for the changes, I've just added comments.

I feel like maybe you missed a few of my previous comments so be sure to check them before we can move on.

Thank you

lib/connectors/mysql-connector.ts

bleugateau · 2021-06-23T19:33:47Z

Done :)

bleugateau and others added 4 commits June 2, 2021 22:36

MysqlConnector: Adding idleTimeout property

8b7731f

MysqlConnector: Adding reconnect behavior during query when ResponseT…

c1976bb

…imeoutError happened

MysqlConnector: Update mysql-connector.ts

5bc4427

MySQLConnector: Fix small mistake during reconnection behavior

0d958d9

eveningkid requested changes Jun 4, 2021

View reviewed changes

MySQLConnector: Refactor many things

ab81450

bleugateau force-pushed the master branch from 82b10a5 to ab81450 Compare June 16, 2021 21:57

eveningkid requested changes Jun 16, 2021

View reviewed changes

bleugateau force-pushed the master branch 2 times, most recently from a03f051 to be20558 Compare June 19, 2021 10:00

MySQLConnector: Deno fmt

d70406d

bleugateau force-pushed the master branch from be20558 to d70406d Compare June 23, 2021 19:25

Merge branch 'master' into master

fd56150

eikmei mentioned this pull request Apr 25, 2023

Auto reconnection to the database server is not triggered #257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Auto reconnection to the database server is not triggered #269

fix: Auto reconnection to the database server is not triggered #269

bleugateau commented Jun 3, 2021

eveningkid left a comment

eveningkid Jun 4, 2021

eveningkid Jun 4, 2021

eveningkid Jun 4, 2021

eveningkid Jun 4, 2021 •

edited

eveningkid Jun 4, 2021

eveningkid Jun 4, 2021

bleugateau commented Jun 4, 2021 •

edited

bleugateau commented Jun 9, 2021

bleugateau commented Jun 16, 2021 •

edited

eveningkid left a comment

bleugateau commented Jun 23, 2021 •

edited

fix: Auto reconnection to the database server is not triggered #269

Are you sure you want to change the base?

fix: Auto reconnection to the database server is not triggered #269

Conversation

bleugateau commented Jun 3, 2021

eveningkid left a comment

Choose a reason for hiding this comment

eveningkid Jun 4, 2021

Choose a reason for hiding this comment

eveningkid Jun 4, 2021

Choose a reason for hiding this comment

eveningkid Jun 4, 2021

Choose a reason for hiding this comment

eveningkid Jun 4, 2021 • edited

Choose a reason for hiding this comment

eveningkid Jun 4, 2021

Choose a reason for hiding this comment

eveningkid Jun 4, 2021

Choose a reason for hiding this comment

bleugateau commented Jun 4, 2021 • edited

bleugateau commented Jun 9, 2021

bleugateau commented Jun 16, 2021 • edited

eveningkid left a comment

Choose a reason for hiding this comment

bleugateau commented Jun 23, 2021 • edited

eveningkid Jun 4, 2021 •

edited

bleugateau commented Jun 4, 2021 •

edited

bleugateau commented Jun 16, 2021 •

edited

bleugateau commented Jun 23, 2021 •

edited