Skip to content

Ability to remove handlers, and ability to add FnOnce handlers instead of FnMut. #130

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

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

Conversation

USSURATONCACHI
Copy link

@USSURATONCACHI USSURATONCACHI commented Mar 29, 2025

Changes:

  1. Added set_handler_once(), try_set_handler_once() that accept FnOnce() -> T instead of FnMut. Those variants return whatever FnOnce returns via thread handle.
  2. Made all handler setting function return JoinHandle<_> in order to be able to track threads, and get values from FnOnce.
  3. Added function remove_all_handlers(). It deinitializes the OS signal handler and stops all awaiting ctrl+c handlers. FnOnce, after running once, automatically calls remove_all_handlers().

Also, added a test tests/main/deinit.rs that shows and tests this behaviour.

Reasons, why I propose to add this:

  • Had a usecase where I needed an interrupt handler to:
    • use tokio::sync::oneshot::Channel just once.
    • to separately track two sequential interrupts.
    • to be able to kill the program the usual way, via Ctrl + C, after being done with this crate interrupts.

Even if this PR will not be approved - I would like to get some feedback.

@USSURATONCACHI
Copy link
Author

@Detegr I have noted, that its been a while, since maintainers interacted in Issues or PRs. I would ask for you, or one of other maintainers (if any) to take a look at the latest PRs. I am pinging you here just in case the issue being here is not getting notifications. Best regards.

@Detegr
Copy link
Owner

Detegr commented Mar 31, 2025

Hi.

Yes, this is unfortunately the case. This project is one of the classic cases where a hobby library suddenly gets a lot of users and I, being a full time empolyee and a father of two kids, don't really have the time and the interest required to keep up with it.

I have also intentionally rejected adding more complexity to the crate because I feel responsible for keeping it clean for possible unsafety and/or crashes.

That being said, I'll try to go through the PRs this friday where I should have some free time. No promises.

@Detegr
Copy link
Owner

Detegr commented Apr 4, 2025

Thanks for the contribution. I skimmed through the code and generally it looks good. This use case however isn't worth adding 300+ lines of new code in my opinion. I would rather just implement the function that allows setting the handler back to a default one (see #106 for context).

@USSURATONCACHI
Copy link
Author

USSURATONCACHI commented Apr 4, 2025

Basically, unsafe fn deinit_os_handler() -> Result<(), Error> is that function.

To be fair:

  • ~110 lines of new code are just highlevel set_handler variants that accept FnOnce instead of FnMut. Those have the same code that was in the library already, just with types changed.
  • 93 more lines are only a test.

The rest are the:

  • deinit_os_handler implementations,
  • block_ctrl_c being able to differ failure from handler being removed,
  • Making the windows implementation a bit more reliable.

@Detegr
Copy link
Owner

Detegr commented Apr 7, 2025

Basically, unsafe fn deinit_os_handler() -> Result<(), Error> is that function.

Right, I missed the actual platform implementations the first time skimming through the code. Can you explain me how this approach is better than exposing the deinit_os_handler function to the users? With that you could track the number of times the handler is called with an atomic integer variable and then call deinit_os_handler when you want to revert back to the default signal handling behavior?

@USSURATONCACHI
Copy link
Author

USSURATONCACHI commented Apr 7, 2025

Is is already exposed via:
src/lib.rs, function pub fn remove_all_handlers() -> Result<(), Error>.

That function is a safe wrapper that calls platform-defined deinit_os_handler.

With that you could track the number of times the handler is called with an atomic integer variable and then call deinit_os_handler

You could already do that with regular FnMut handlers + remove_all_handlers().

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.

2 participants