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

Add reap functionality #187

Merged
merged 4 commits into from
Jul 27, 2024
Merged

Conversation

ttstarck
Copy link
Collaborator

@ttstarck ttstarck commented Jul 23, 2024

There have been several discussions/attempts at adding a idle connection reaping functionality to this gem: #125 #127 #126

I and many others would find value in adding this functionality however I am in agreement with the previous discussions that the key to adding this reap functionality is to add it in a way that keeps the simplicity of this gem.

In order to accomplish this, I've added two new public methods to ConnectionPool (as well as matching methods for TimedStack):

  1. #idle - Returns how many connections that are created and available in the pool. This is different than available because available includes connections that haven't been created yet.
  2. #reap(idle_seconds = 0, &block) - Removes connections that have been idle for longer than idle_seconds and yields each connection to the block passed to the method in order to close/clean up the connection. By default, idle_seconds is 0, which means #reap will remove all connections that are on the stack.

In order to implement the #reap method in connection pool, I've changed how the TimedStack queue stores connections in the queue to also include the current time when pushed back onto the stack.

@ttstarck ttstarck marked this pull request as ready for review July 23, 2024 20:03
@mperham
Copy link
Owner

mperham commented Jul 24, 2024

I’m wondering if the default idle_seconds value should be something more like 60. 0 is very aggressive.

0 seconds is on the aggresive, setting to a more sane value of 60 seconds to prevent connection reaping/creation thrash.
@ttstarck
Copy link
Collaborator Author

ttstarck commented Jul 24, 2024

I’m wondering if the default idle_seconds value should be something more like 60. 0 is very aggressive.

Good call, 60 seconds is a more sane value such that this method can be called and not cause thrash of opening/closing connections.
Fixed c4f5a6c

@mperham
Copy link
Owner

mperham commented Jul 25, 2024

Did you document how the user can implement reaping?

@ttstarck
Copy link
Collaborator Author

Did you document how the user can implement reaping?

Updated the README with information on using #reap and #idle methods: 2148a28

I could also add a section on implementing a reaper thread 🤔... Is that what you are referring to or is what I added above enough.

@mperham
Copy link
Owner

mperham commented Jul 25, 2024

Yeah, I meant with a Thread. Your code example isn't quite realistic yet, I'd change it to put the reap call in a Thread.

@ttstarck
Copy link
Collaborator Author

Sorry for the delay:

I've added a new section about creating your own reaper thread to the README:

28e9760

@mperham
Copy link
Owner

mperham commented Jul 27, 2024

This is great, nice work. I’ll play with it later and push out a new release in a few weeks.

@mperham mperham merged commit 062500b into mperham:main Jul 27, 2024
9 checks passed
@ttstarck ttstarck deleted the add_reap_functionality branch July 27, 2024 22:36
Comment on lines +179 to +180
conn, _last_checked_out = @que.shift
reap_block.call(conn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this also need to decrement @created?

@shayonj
Copy link
Contributor

shayonj commented Aug 15, 2024

Excited for this, thank you!

@mperham
Copy link
Owner

mperham commented Jan 7, 2025

2.5.0 is now out with reaping functionality.

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.

4 participants