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

fix: Close host on SIGINT #2829

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

Conversation

MarcoPolo
Copy link
Contributor

Closes #2785

This is closer to the old behavior prior to V0.34.

The first SIGINT closes the libp2p host. The second SIGINT follows the default Go behavior, unless the user is handling it.

This means, unless the user does something different, the default behavior will be 2 ctrl-c's are necessary to exit the program. I don't think this is perfect, but it's best we can do since FX will handle the first ctrl-c, and I don't think we should try to replicate the default Go behavior there. For now, I've added a log.Info to provide some info.

I'm open to other suggestions here.

@MarcoPolo MarcoPolo requested a review from sukunrt June 10, 2024 18:29
Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

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

Alternatively, we can expose the app.Done() channel and let users decide what to do with the signal. Users with more context can just call os.Exit after stopping the host and exit the program.

If I understand correctly previously, the default go signal handler terminated the program which users could override by registering their own signal handler.
If we make this change, users cannot override the behavior of stopping on ctrl+c

@richard-ramos
Copy link
Contributor

I like the idea of exposing app.Done(), because then it would be possible to implement at the app level a behavior more natural of interrupting the app on a single SIGINT, compared to the proposal of having to press it twice

@MarcoPolo
Copy link
Contributor Author

I think the real solution here is if fx didn't register signal handlers at all. This would allow users to:

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.

Process unresponsive to SIGINT
3 participants