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

Keep alive #1081

Merged
merged 3 commits into from
Dec 13, 2019
Merged

Keep alive #1081

merged 3 commits into from
Dec 13, 2019

Conversation

benbotto
Copy link
Contributor

@benbotto benbotto commented Dec 13, 2019

This PR allows the user to enable keep-alive on the underlying socket with a configurable initialDelay parameter, in milliseconds. Keep-alive is disabled by default. Example usage:

const pool = mysql.createPool({
  host: 'db',
  user: process.env.MYSQL_USER,
  password: process.env.MYSQL_PASSWORD,
  database: process.env.MYSQL_DATABASE,
  connectionLimit: 2,
  waitForConnections: true,
  queueLimit: 0,
  keepAliveInitialDelay: 10000, // 0 by default.
  enableKeepAlive: true, // false by default.
});

@sidorares
Copy link
Owner

I wonder if it worth making keepAlive on by default. If it won't add perf degradation and regression overall it'll potentially make life easier for many
At the same time default off is safer and we can change this behaviour later

@sidorares
Copy link
Owner

Also if it's default off and not mentioned in read me there is 0 chance it'll be used anywhere in prod

@benbotto
Copy link
Contributor Author

It's up to you. I can update the PR if you want, but I wanted to err on the side of caution.

@benbotto
Copy link
Contributor Author

There are some other issues that talk about ECONNRESET.

#897
#907
#128

In the first one the reporter is using Docker Swarm, so I'm pretty sure it's the same issue I was running up against.

At any rate, I wonder if there are some other members of the community that could test this change by enabling keep-alive manually using the new options in this PR. Then the option could be enabled by default if it's viewed as generally beneficial.

@sidorares
Copy link
Owner

Yeah, let's soft launch it first and I'll just advise all similar reports to try keepAlive option, later we can document and enable on by default

@sidorares sidorares merged commit f9b9f96 into sidorares:master Dec 13, 2019
@midnightcodr
Copy link

@sidorares can you up the npm version so the enableKeepAlive and keepAliveInitialDelay options can be used? Thanks.

@sidorares
Copy link
Owner

after some thought - maybe .setKeepAlive() should have been added only when we create tcp/pipe connection instead of detecting if method is available on stream (after

line).

@benbotto
Copy link
Contributor Author

I mentioned this as an aside over in #683, but I'm not up to speed on the four branches in the code referenced above (lines 31-45). There are 4 ways that this.stream gets set:

  1. connecting over a unix socket;
  2. connection via TCP;
  3. a user-supplied stream;
  4. or a factory function to produce a stream.

Is my understanding of (3) and (4) accurate? And in those cases, since the user is creating the stream, is it essentially up to them to enable keep-alive if they want to use it?

@sidorares
Copy link
Owner

@benbotto yes, all correct

@sidorares
Copy link
Owner

option 4 was added later when I realised that pool needs a way to know how to create new underlying connection and it was too late to remove option 3

@benbotto
Copy link
Contributor Author

Cool, thanks for the clarification. I made the changed and added a new PR: #1086

@jsumners
Copy link

jsumners commented Jan 9, 2020

FYI: documentation should have been updated in this PR.

franck-nadeau pushed a commit to franck-nadeau/mysql that referenced this pull request Jan 14, 2020
The following [PR](sidorares/node-mysql2#1081) added two new options to mysql's Pool creation.

I am simply adding them to the library in this PR.
unional pushed a commit to types/mysql that referenced this pull request Jan 14, 2020
The following [PR](sidorares/node-mysql2#1081) added two new options to mysql's Pool creation.

I am simply adding them to the library in this PR.
@DigitalSolomon
Copy link

Yeah, let's soft launch it first and I'll just advise all similar reports to try keepAlive option, later we can document and enable on by default

This deserves to be on the front page of the documentation. Took a lot of traveling down various rabbit holes to come upon this relatively simple solution. Would be great if this showed up on https://www.npmjs.com/package/mysql2 as well.

@Kennytian
Copy link

SET GLOBAL wait_timeout = 60;

run this command in mysql client command terminal, the timeout unit is seconds

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.

6 participants