feat: gracefully end pool connections #3148

[dstankovd]

Related to: #3148

When Pool._removeIdleTimeoutConnections() is executed and an idle connection is selected for removal (regardless of the conditions) the stream is just ended without notifying the MySQL server. This leads to warnings being logged in the SQL error log.

This pull requests add additional config option that would gracefully close the pool connection.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.83%. Comparing base (349f870) to head (2fa30f2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3776      +/-   ##
==========================================
+ Coverage   89.25%   89.83%   +0.57%     
==========================================
  Files          86       86              
  Lines       13583    13596      +13     
  Branches     1584     1606      +22     
==========================================
+ Hits        12124    12214      +90     
+ Misses       1459     1382      -77     

Flag	Coverage Δ
compression-0	88.94% <100.00%> (+0.59%)	⬆️
compression-1	89.81% <100.00%> (+0.57%)	⬆️
static-parser-0	87.40% <100.00%> (+0.57%)	⬆️
static-parser-1	88.17% <100.00%> (+0.57%)	⬆️
tls-0	89.25% <100.00%> (+0.57%)	⬆️
tls-1	89.60% <100.00%> (+0.57%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wellwelwel]

Thanks, @dstankovd! After the test/integration/test-pool-end.test.cjs test, the connection is not being closed. By order of execution, it's probably the test-pool-release-idle-connection-replicate.test.cjs test.

[dstankovd]

Thanks, @dstankovd! After the test/integration/test-pool-end.test.cjs test, the connection is not being closed. By order of execution, it's probably the test-pool-release-idle-connection-replicate.test.cjs test.

I'm trying to figure out the hanging test runs, but I'm having trouble reproducing this issue.

On my local machine, the full test suite runs successfully against MySQL v8.3.0. I also tried using deno with the setup from this failing job, and all tests completed without hanging. Since the tests pass in a replicated environment, do you have any thoughts on what could be causing this flakiness in the CI?

As a side note, I did find some tests that hangwhen using MariaDB 11.4. I believe this is an unrelated issue that's not connected to this PR, and I'll look into it separately.

[trungdq88]

@dstankovd @wellwelwel first of all thank you for your time in maintaining this project!

After reading on this issue, I think the previous PR https://github.com/sidorares/node-mysql2/pull/3180/files is a much better solution. Much cleaner and avoid another confusing gracefulEnd flag (especially the flag changes the behavior of the end method as described here).

Any chance we can get @sidorares approval on that instead? My database log is also flooded with the "aborted connection" error messages so I would love to have this issue fixed.

Thanks!

[wellwelwel]

@dstankovd, just checking and confirming that the error is in the new test (test/integration/test-pool-release-idle-connection-graceful-end.test.cjs).

@trungdq88, I'm fine with either solution, but since the previous one removes a depreciation, it's interesting to consider @sidorares's point, especially for architectural reasons. Whereas in this current solution, since it's a new flag, it is more flexible to be merged before.

[dstankovd]

Hey @wellwelwel, finally managed to reproduce the issue locally and I've reorganized the new tests a bit and changed them to depend on process exit to validate the outcome, that appears to have fixed the test execution for me. Hopefully the CI will pass too. :)

As for the @trungdq88's suggestion - I also prefer the solution in the previous pull request but, if the compatibility with mysql is a must, then it's a no-go. Generally, I don't think that changing the end method in the pool_connection would have led to any negative impact since it has been deprecated for a few years now and it does come with a big warning, so it's unlikely that anyone uses it.

Either way, for now I'm keen to have the logs clear from the "aborted connection" warning so I'd gladly use the work-around implementation in this pull request. I can open a new issue to further discuss the possibility of actually changing the end method.

[wellwelwel]

Updating just to bring in the #1752 changes.

Regarding changing the end method itself, we can revisit this point when a major version is under discussion.

[wellwelwel]

Thanks again, @dstankovd! It's already available in the mysql2@canary version and will soon be available in the main version as well 🤝