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

ECONNRESET when closing SSL connection not being handled to NodeJS Error spec #1543

Open
kpervin opened this issue Mar 25, 2022 · 7 comments

Comments

@kpervin
Copy link

kpervin commented Mar 25, 2022

Hi there, I receive the following error when closing a SSL connection that bubbles up:

{
  errno: -4077,
  code: "ECONNRESET",
  syscall: "read",
  stack:
    "Error: read ECONNRESET\n    at TLSWrap.onStreamRead (node:internal/stream_base_commons:220:20)\n    at TLSWrap.callbackTrampoline (node:internal/async_hooks:130:17)",
  message: "read ECONNRESET",
}

I've narrowed it down to the following lines that are mishandling the exception thrown:

// Line 160 of mysql2/lib/connection.js
 _handleNetworkError(err) {
    if (this.connectTimeout) {
      Timers.clearTimeout(this.connectTimeout);
      this.connectTimeout = null;
    }
    // Do not throw an error when a connection ends with a RST,ACK packet
    if (err.errno === 'ECONNRESET' && this._closing) {
      return;
    }
    this._handleFatalError(err);
  }

Issue seems to be that it's looking for err.errno as a string. However, errno has exclusively been a negative number since Node 13, with err.code being the string that would emit ECONNRESET.

I see that this has been fixed in master. Can this please be released to npm?

@kpervin kpervin changed the title ECONNRESET when closing SSL connection ECONNRESET when closing SSL connection not being handled to NodeJS Error spec in v13+ Mar 27, 2022
@kpervin kpervin changed the title ECONNRESET when closing SSL connection not being handled to NodeJS Error spec in v13+ ECONNRESET when closing SSL connection not being handled to NodeJS Error spec in version >=13 Mar 27, 2022
@kpervin kpervin changed the title ECONNRESET when closing SSL connection not being handled to NodeJS Error spec in version >=13 ECONNRESET when closing SSL connection not being handled to NodeJS Error spec Mar 27, 2022
@kpervin
Copy link
Author

kpervin commented Mar 31, 2022

@sidorares Is the fix planned for release anytime soon to NPM? If not, is there a reason?

@kf6kjg
Copy link

kf6kjg commented Apr 12, 2022

Just met this one myself. Thankfully the patch-package utility exists so I can fix it for my codebase and move on.

Here's my patch: mysql2_2.3.3.patch

diff --git a/node_modules/mysql2/lib/connection.js b/node_modules/mysql2/lib/connection.js
index 47970e9..4704db5 100644
--- a/node_modules/mysql2/lib/connection.js
+++ b/node_modules/mysql2/lib/connection.js
@@ -174,7 +174,7 @@ class Connection extends EventEmitter {
       this.connectTimeout = null;
     }
     // Do not throw an error when a connection ends with a RST,ACK packet
-    if (err.errno === 'ECONNRESET' && this._closing) {
+    if (err.code === 'ECONNRESET' && this._closing) {
       return;
     }
     this._handleFatalError(err);

@kpervin
Copy link
Author

kpervin commented Apr 12, 2022

Just met this one myself. Thankfully the patch-package utility exists so I can fix it for my codebase and move on.

Here's my patch: mysql2_2.3.3.patch

diff --git a/node_modules/mysql2/lib/connection.js b/node_modules/mysql2/lib/connection.js
index 47970e9..4704db5 100644
--- a/node_modules/mysql2/lib/connection.js
+++ b/node_modules/mysql2/lib/connection.js
@@ -174,7 +174,7 @@ class Connection extends EventEmitter {
       this.connectTimeout = null;
     }
     // Do not throw an error when a connection ends with a RST,ACK packet
-    if (err.errno === 'ECONNRESET' && this._closing) {
+    if (err.code === 'ECONNRESET' && this._closing) {
       return;
     }
     this._handleFatalError(err);

That looks useful. I'll have to look into it further. Thanks for the heads up!

@sidorares
Copy link
Owner

@kpervin @kf6kjg the fix should be in master ( #1438 ) but is not published yet ( not sure why, last release published after PR was merged. need to investigate that )

@sidorares
Copy link
Owner

would be good to add an integration test for this

@kpervin
Copy link
Author

kpervin commented May 10, 2022

@kpervin @kf6kjg the fix should be in master ( #1438 ) but is not published yet ( not sure why, last release published after PR was merged. need to investigate that )

Any news regarding this being published to NPM?

@kpervin
Copy link
Author

kpervin commented May 25, 2022

@sidorares Will this be pushed to NPM? At present I am having to patch the repo via yarn patch in the meantime.

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

3 participants