Skip to content

Add on_thread_exit hook #2920

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 11, 2023
Merged

Add on_thread_exit hook #2920

merged 2 commits into from
Jul 11, 2023

Conversation

biinari
Copy link
Contributor

@biinari biinari commented Aug 30, 2022 •
edited
Loading

Closes #2100

Description

For apps that need to tidy up thread-local resources when a thread exits, add a new on_thread_exit hook.
This will be called when a thread is being trimmed, just before the thread exits.

An example config entry involving rabbitmq channels being held in thread local variables (one channel per thread):

on_thread_exit do
  unless Thread.current[MY_KEY].nil?
    Thread.current[MY_KEY].disconnect
    Thread.current[MY_KEY] = nil
  end
end

The hook should be configured with a quick non-blocking operation as it will block new requests from being started in other threads. I wondered about moving the call to the hook outside of the mutex section, but that becomes quite a bit less readable and I can't see what anyone would need to do that would be a blocking operation.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@nateberkopec nateberkopec added feature waiting-for-review Waiting on review from anyone labels Aug 31, 2022
@dentarg
Copy link
Member

dentarg commented Sep 27, 2022

LGTM 👌 Needs a rebase though.

One thought, should we call it before_thread_exit_hooks? As you can have multiple hooks. I guess you followed out_of_band_hook which also could use an s on the end IMHO.

@MSP-Greg
Copy link
Member

Should there be a hook on the front end, immediately after the thread block starts?

Also, current code in this PR rescues outside of before_thread_exit_hook.each(&:call). With other hooks it's done inside the iteration, so if one hook errors, the others are still called.

@biinari
Add on_thread_exit hook
a43167f 
Add a hook to run when a worker thread is trimmed (exits normally).
This can be useful to clean up thread local resources that do not want to be
cleaned between every request (see clean_thread_locals for that).
@biinari
Copy link
Contributor Author

biinari commented Oct 2, 2022

Thanks for the suggestions. I've rebased and followed the naming from the out_of_band which now has no _hook suffix in the instance variable.

@MSP-Greg, it sounds like you're asking for an additional feature with a hook at the start of running a thread. If you have a use case for needing this, perhaps create a separate issue? (Or correct me if I've misunderstood).

Changed the rescue to be per hook call as suggested, that's a good idea.

@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 2, 2022 •
edited
Loading

@biinari

to tidy up thread-local resources

Not an additional feature, but possibly a more complete implementation of the feature. In this case, a logical place to create the 'thread-local resources' might be a companion on_thread_start (or on_thread_enter) hook.

Edit: I'll defer to others on this one, so maybe wait for their input. I just want to avoid someone asking for it at a later time.

@biinari
Copy link
Contributor Author

biinari commented Oct 3, 2022

@MSP-Greg, actually that does sound sensible to add in with this as it would be a place to create thread-local resources or to monitor when threads are starting and stopping. I'll take a look at it soon assuming nobody disagrees.

@nateberkopec
Copy link
Member

@biinari Works for me too, please do investigate 👍

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Oct 20, 2022
@nateberkopec
Copy link
Member

It looks like OP needed to move on to other things.

I don't think adding on_thread_start should actually block this though? We can merge as-is and make adding the complementary hook a separate issue.

@nateberkopec nateberkopec merged commit db06025 into puma:master Jul 11, 2023
@nateberkopec nateberkopec mentioned this pull request Jul 11, 2023
Closed
@biinari
Copy link
Contributor Author

biinari commented Jul 11, 2023

Thanks for merging this in. Sorry you're right, I've had my focus other projects for a while.

@nateberkopec
Copy link
Member

No worries homie, no one's on a timeline here 👍 Thanks for your contribution 🙇

@binarygit binarygit mentioned this pull request Jul 17, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How can I know when the thread is removed from the pool?
4 participants
@biinari @dentarg @MSP-Greg @nateberkopec