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

fix: gracefully close pool connections #3180

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dstankovd
Copy link

@dstankovd dstankovd commented Nov 4, 2024

Fixes #3148

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.30%. Comparing base (cdc9415) to head (acd8b70).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
+ Coverage   88.23%   88.30%   +0.06%     
==========================================
  Files          83       83              
  Lines       12985    12975      -10     
  Branches     1373     1373              
==========================================
  Hits        11457    11457              
+ Misses       1528     1518      -10     
Flag Coverage Δ
compression-0 88.30% <100.00%> (+0.06%) ⬆️
compression-1 88.30% <100.00%> (+0.06%) ⬆️
tls-0 87.69% <100.00%> (+0.06%) ⬆️
tls-1 88.06% <100.00%> (+0.06%) ⬆️

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.

@wellwelwel
Copy link
Collaborator

Thanks, @dstankovd 🙋🏻‍♂️

Do you think a test can be added to cover these changes?

@wellwelwel wellwelwel changed the title fix: gracefully close pool connections sidorares#3148 fix: gracefully close pool connections Nov 5, 2024
@dstankovd
Copy link
Author

Thanks, @dstankovd 🙋🏻‍♂️

Do you think a test can be added to cover these changes?

Hey, I've added a new test covering theses changes :)

@wellwelwel
Copy link
Collaborator

wellwelwel commented Nov 14, 2024

@dstankovd, in #3081 we moved the lib/pool.js and lib/pool_connection.js to lib/base/pool.js and lib/base/pool_connection.js. This led to the need for a rebase.

Sorry for the inconvenience 🙋🏻‍♂️

Also, thanks for the tests 🤝

@dstankovd dstankovd force-pushed the gracefull-pool-connection-close branch from 39148c6 to 9201470 Compare November 14, 2024 16:41
@dstankovd
Copy link
Author

Hey @wellwelwel. I've rebased the branch, not sure why there are failed checks tho..

@wellwelwel
Copy link
Collaborator

wellwelwel commented Nov 14, 2024

@dstankovd, I don't know if it's a bug or if it's intentional, but it seems that Deno has completely removed support for local CommonJS in the canary version.

In any case, the failed workflows are unrelated to your changes 🙋🏻‍♂️


Edit: Not a bug, but a breaking change (already fixed in #3209).

@wellwelwel
Copy link
Collaborator

I've rebased the branch, not sure why there are failed checks tho..

@sidorares, could you enable the following option in the repository settings?

Screenshot 2024-11-14 at 21 12 49
  • This will display a button that allows us to automatically rebase outdated branches without conflict.

@dstankovd dstankovd force-pushed the gracefull-pool-connection-close branch from c9ecc44 to acd8b70 Compare November 16, 2024 19:07
@dstankovd
Copy link
Author

Hey @wellwelwel ,

rebased again, so all checks should pass now :)

Copy link

@dygabo dygabo left a comment

Choose a reason for hiding this comment

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

Hello, any plans on when will this fix be released?

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.

Pool connections are not gracefully closed once idle connection cleanup occurs.
4 participants