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 possibly hanging process after SIGINT/SIGTERM. #292

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

s-ludwig
Copy link
Member

Ensures that the thread local event loop gets terminated, even if it is currently in an infinite wait state.

Fixed vibe-d/vibe.d#2596

Ensures that the thread local event loop gets terminated, even if it is currently in an infinite wait state.

Fixed vibe-d/vibe.d#2596
@Geod24
Copy link
Contributor

Geod24 commented Aug 24, 2021

Tested, confirmed fixed, thanks!

@Geod24 Geod24 merged commit 08f95b3 into master Aug 24, 2021
@Geod24 Geod24 deleted the fix_hang_after_sigint branch August 24, 2021 10:03
@Geod24
Copy link
Contributor

Geod24 commented Aug 24, 2021

@s-ludwig : Not wanting to move the goal post, but since we're on the topic of shutdown:

Currently, it seems that Vibe do not shutdown listeners itself. Most of our examples use things like listenHTTP, e.g. the app_skeleton, but we should actually be recommending auto listener = listenHTTP(...); so that the user can shut down the connection.

However, even that is not enough. If one really want to get rid of the shutdown messages, one has to disable the default signal handlers and provide their own implementation. For some, it is necessary anyway, as they might need to perform some non-Vibe-related cleanup on shutdown. See our example, where we need to call the node's shutdown() procedure.

It's not a blocking issue per se (it's possible to do what we want with the current API), but I feel the API fails to address a common, and IMO basic, need. There's a few ways this could be addressed, but I was wondering first if you were aware of this / had some thoughts on it ?

@s-ludwig
Copy link
Member Author

I had this topic on my mind for some time and I definitely agree with the auto listener = listenHTTP((...). But since we don't do the shared static this approach anymore, I think that we don't really have to change the shutdown procedure itself that much* or add new APIs. It should be sufficient to simply move any shutdown procedures to after the runApplication/runEventLoop call.

* What comes to mind is that currently the event loops of all threads are exited, whereas only the main one should be. The main thread would then have to make sure that other threads are shut down properly.

@Geod24
Copy link
Contributor

Geod24 commented Aug 25, 2021

Hum, I'll give it a shot in our app, to see if that currently works.

But since we don't do the shared static this approach anymore [...]

Well if I ever get free time I'll definitely look at vibe-d/vibe.d#2370 then :)

This pull request was closed.
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.

Basic example creates zombie on Mac OS
2 participants