search

sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
4.02k stars 606 forks source link

keepalive not behaving as documented #1586

Open mrx8 opened 2 years ago

mrx8 commented 2 years ago

according to the commit: https://github.com/sidorares/node-mysql2/commit/6b59a3604a53a7f85c8392fcb9577aabbd1eb4df keepalive is now always on. It cannot be disabled. The inline-documentation erroneously says it is off by default. I have a problem with this in combination with the libkeepalive-library and need to disable it. Is it possible to integrate the possibility to disable keepalive in mysql2 once more ?

sidorares commented 2 years ago

hm, the change looks quite out of sync with the comments, I might need to re-read communication leading to in https://github.com/sidorares/node-mysql2/pull/1081

@benbotto can you comment?

benbotto commented 2 years ago

I think this is a bug--true shouldn't be hard coded in the call to setKeepAlive. Seems like #1081 had the right logic, but in #1086 I erroneously removed the if (this.config.enableKeepAlive) check.

1258 seems to correct the issue, mostly, but there's a comment on Pool.d.ts about a bad type. Also might want a !! on https://github.com/sidorares/node-mysql2/pull/1258/files#diff-1978d11697f58179bc32b66c6ac7f21cbdfd6649a8f013248a1f9feee25858feR99

    this.enableKeepAlive =
      options.enableKeepAlive === undefined || !!options.enableKeepAlive;

Not sure why #1258 was closed.

sidorares commented 2 years ago

@kn3ny are you keen to finish that work? The PR looks mostly ready, happy to merge it but if you still plan to make a fresh one let us know

n0v1 commented 1 year ago

I've just created PR #2016 to fix this.