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 return handle #110

Closed
wants to merge 1 commit into from
Closed

Add return handle #110

wants to merge 1 commit into from

Conversation

ssrlive
Copy link

@ssrlive ssrlive commented Oct 5, 2023

Fix #90

@ssrlive
Copy link
Author

ssrlive commented Oct 12, 2023

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

@Detegr
Copy link
Owner

Detegr commented Oct 25, 2023

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
Copy link
Author

ssrlive commented Oct 25, 2023

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
Copy link
Owner

Detegr commented Oct 25, 2023

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
Copy link
Author

ssrlive commented Oct 25, 2023

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
Copy link
Author

ssrlive commented Oct 26, 2023

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

@Detegr
Copy link
Owner

Detegr commented Oct 26, 2023

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.

@Detegr Detegr closed this Oct 26, 2023
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.

Possible memory leak?
2 participants