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

Update how the ECONNRESET error is caught when connection already closing #1438

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

kneemer
Copy link
Contributor

@kneemer kneemer commented Oct 28, 2021

Hello 👋 ,

I was having a problem similar to #766 while moving our MySQL database from AWS to Azure. I would get a successful response AND an ECONNRESET error when querying the Azure database.

The fix for #766 didn't seem to be working for me. After digging in a bit, I saw that the error I was getting had a number for the errno property and not the ECONNRESET string:

Error: read ECONNRESET
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:209:20)
    at TLSWrap.callbackTrampoline (internal/async_hooks.js:131:14) {
  errno: -54,
  code: 'ECONNRESET',
  syscall: 'read',
  fatal: true
}

It looks like the errno property changed in node v13

The error.errno property is a negative number which corresponds to the error code defined in libuv Error handling.

Previously, in node v12:

The error.errno property is a number or a string. If it is a number, it is a negative value which corresponds to the error code defined in libuv Error handling. ... In case of a string, it is the same as error.code.

I believe changing err.errno to err.code is backwards compatible given the description in the node v12 docs. It also follows how this error is caught in mysqljs/mysql

If I apply this change locally, our application behaves much better. I attempted to add a test before opening this PR as there didn't seem to be coverage but was not successful. Hopefully the provided documentation and explanation will be enough. 🤞

For additional context, I ran a simple query using our sequelize setup and NODE_DEBUG=net npm run console to see the sequence of events.
Screen Shot 2021-10-28 at 2 59 17 PM

@sidorares
Copy link
Owner

Thanks @kneemer !

I attempted to add a test before opening this PR as there didn't seem to be coverage but was not successful

I can try to guide you. While the change looks straightforward I'd really like to have it covered with unit test. For this scenario we probably need to use fake server in order to trigger ECONNRESET on an underlying tcp connection

( ignore failing coverage check for now, it's a known issue )

@kneemer
Copy link
Contributor Author

kneemer commented Oct 29, 2021

I can try to guide you. While the change looks straightforward I'd really like to have it covered with unit test. For this scenario we probably need to use fake server in order to trigger ECONNRESET on an underlying tcp connection

That makes sense, if you point me to a particular test to emulate I could start there.

@sidorares
Copy link
Owner

});

process.on('uncaughtException', err => {
assert.notEqual(err.code, 'ECONNRESET')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails if I revert the err.code back to err.errno. I made the assertion targeted so this doesn't accidentally test some other error condition.

@kneemer
Copy link
Contributor Author

kneemer commented Nov 1, 2021

@sidorares let me know if this test is the right approach or not. If not, any additional guidance would be welcomed. 😄

@sidorares
Copy link
Owner

All good with the test. I'll try to fix ci today, you might need to rebase or merge master into you branch after

@sidorares sidorares merged commit 686a56e into sidorares:master Nov 24, 2021
@kpervin
Copy link

kpervin commented Apr 6, 2022

@sidorares Is there any update on when this might get uploaded to NPM? This package is a dependency of MikroORM and at present is a bit of a breaking issue for environments using Node 13+.

@sidorares
Copy link
Owner

@kpervin thanks for the ping, somehow this got lost, I'll try to put some attention

@sidorares
Copy link
Owner

actually this should be published. Can you test with the latest v2.3.3 @kpervin ?

@kpervin
Copy link

kpervin commented Apr 7, 2022

@sidorares Unfortunately, after looking at the 2.3.3 package in my node_modules, it still seems to be apparent.

image

I also looked at the 2.3.3 tagged release, and that line seems to be the same.

@daniellockyer
Copy link
Contributor

daniellockyer commented Jul 4, 2022

Ping on this after receiving a report in Ghost.

@sidorares Anything I can do to help? Looks like we need the change releasing 🙂

@kpervin
Copy link

kpervin commented Jul 4, 2022

Ping on this after receiving a report in Ghost.

Anything I can do to help? Looks like we need the change releasing 🙂

It's already in master, so it only requires a push to NPM.

@ErisDS
Copy link

ErisDS commented Jul 26, 2022

Curious - is there something blocking the release? There seems to be loads of great changes in master but noone is benefiting from them.

This issue in particular is affecting our (Ghost's) users so it would be fab if we could get it onto npm.

@ErisDS ErisDS mentioned this pull request Sep 8, 2022
daniellockyer added a commit to TryGhost/Ghost that referenced this pull request Jan 12, 2023
fixes #14990

- there was a bug in `mysql2` [1] when connecting to Azure DBs, but this was
  subsequently fixed, so this commit bumps the package in Ghost and
  `knex-migrator`, where this was also bumped
- of note, this release includes sidorares/node-mysql2#1666 and
  sidorares/node-mysql2#1751, which are very interesting

[1]: sidorares/node-mysql2#1438
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

Successfully merging this pull request may close these issues.

5 participants