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

Track checked out created connections across reloads #192

Conversation

ttstarck
Copy link
Collaborator

@ttstarck ttstarck commented Aug 14, 2024

There appears to be a small bug around ConnectionPool#reload where it is possible to create more connections than the size of the pool if you run reload while a connection is checked out.

See the test cases defined in 4f4e5f6

These test cases all fail on the master branch. This is due to how @created instance variable in ConnectionPool::TimedStack is decremented, where currently it is set to 0 on #shutdown(reload: true) even if no connections were "shutdown" on the reload.

@ttstarck ttstarck changed the title Track checked out created connectionss through reloads Track checked out created connectionss across reloads Aug 14, 2024
@ttstarck ttstarck changed the title Track checked out created connectionss across reloads Track checked out created connections across reloads Aug 14, 2024
@ttstarck ttstarck force-pushed the track_checked_out_created_conns_through_shutdowns branch from 91b737c to 778310d Compare August 14, 2024 22:15
@ttstarck
Copy link
Collaborator Author

ttstarck commented Aug 14, 2024

It seems unit tests are failing due to an issue in Minitest v5.25.0, specifically related to this issue: minitest/minitest#1009

There is a fix in the works: minitest/minitest#1010

@zenspider
Copy link

Why did ruby 2.5 pass‽‽

@zenspider
Copy link

The other fix is to put a TERM: Bogus in your workflow's env hash

@mperham
Copy link
Owner

mperham commented Aug 15, 2024

I don’t particularly care for the decrement created method. Could you in-line that please?

@ttstarck ttstarck marked this pull request as ready for review August 17, 2024 00:25
@ttstarck ttstarck requested a review from mperham August 21, 2024 17:53
@mperham mperham merged commit a9ed3e2 into mperham:main Aug 26, 2024
9 checks passed
@mperham
Copy link
Owner

mperham commented Aug 26, 2024

@ttstarck Would you be interested in commit bit here?

@ttstarck
Copy link
Collaborator Author

@mperham I am not sure what you mean by "commit bit", (google shows me https://github.com/cucumber/commitbit). I'd be happy to be elevated to a higher level of permissions.

@ttstarck ttstarck deleted the track_checked_out_created_conns_through_shutdowns branch August 27, 2024 21:28
@mperham
Copy link
Owner

mperham commented Aug 27, 2024

It means you have commit rights to this repo. Everyone still works on PRs for major things but minor things like updating the changelog can be done directly.

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.

3 participants