search

Detegr / rust-ctrlc

Easy Ctrl-C handler for Rust projects
https://crates.io/crates/ctrlc
Other
599 stars 79 forks source link

Add return handle #110

Closed ssrlive closed 1 year ago

ssrlive commented 1 year ago

Fix #90

ssrlive commented 1 year ago

Please review this PR, I will make change if you find any problem.

Detegr commented 1 year ago

The code looks good, considering the approach. It's just that I don't see this as a problem, really. Unless proven otherwise my current feeling is that it's better to leave the thread unjoined than to introduce the burden of returning a boolean from the handler. There were attempts (#60) to provide alternative ways, but I buried them as they introduced quite a lot of complexity (and no one was willing to review the highly unsafe code).

Maybe if this could be done in a way that the return value from the handler isn't a boolean, but some custom type that the unit type () could be automatically converted to (I'm not sure if this is possible, maybe not), then it would feel better as the user wouldn't need to change their existing handlers but could return something like ctrlc::ExitHandler if and only if they wished the handler to be exited.

ssrlive commented 1 year ago

I can't imagine what the return value of user_handler should be designed to be able to exit gracefully, other than a Boolean value.

Perhaps, for some scenarios, the optimal solution is just the one that solves the problem most directly.

For the end user, changing the return value from () to bool is not a completely incomprehensible burden.

Detegr commented 1 year ago

For the end user, changing the return value from () to bool is not a completely incomprehensible burden.

I would agree if the user actually had some benefit of changing it, but this issue is just a theoretical issue. There is only one thread which lasts for the whole duration of the program. There isn't a way to create multiple of these threads, so there is no way to leak memory. Yes, valgrind reports a leak but it is false positive because the thread exists for the whole lifetime of the program.

If you think this is a real issue, please provide an example that demonstrates it. The example in #90 isn't a real memory leak, like I explained.

ssrlive commented 1 year ago

Since the original design caused valgrind's error reporting, if a small change can eliminate this problem, I think the change is worth it.

ssrlive commented 1 year ago

Well, I'll publish ctrlc2 to crates.io. No time to wait you anymore.

Detegr commented 1 year ago

Since the original design caused valgrind's error reporting, if a small change can eliminate this problem, I think the change is worth it.

I disagree.

Well, I'll publish ctrlc2 to crates.io. No time to wait you anymore.

I assume no discussion here is needed anymore then. Closing this PR.