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 manual steps describing how to check launcher behavior regarding POSIX signals #77

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Mar 18, 2019

Issue Number

#8

Overview

  • I have added some manual steps to verify that SIGINT and SIGTERM are handled properly

Comments

It's more about having that written that so that, we won't forget to test it upon releases!

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

This is nice.
In order to make it more complete maybe we could add also cases for:

  • SIGQUIT -> Ctrl + \
  • SIGKILL -> kill -9

cf: https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

@KtorZ
Copy link
Member Author

KtorZ commented Mar 18, 2019

Sure 👍, though, keep in mind that we do not handle the SIGKILL for now. This one is a bit tricky because in practice, we can't put a signal handler since the SIGKILL with forcefully and immediately terminates the process (which is, the expected semantic for a SIGKILL).

We had a chat this morning with @rvl about a way to handle that by having a shared socket between the supervisor process and its children. When the supervisor gets closed, one end of the socket will get closed as well, cascading to the child which will also terminate. For now, I'll mention the current behavior in the steps, and we can refine it later for the SIGKILL if we want to.

@piotr-iohk
Copy link
Contributor

Yes, I noticed some comments about SIGKILL on other PRs later on, after putting this comment.

Just thinking If we won't support SIGKILL than maybe it would be worth putting this in some user facing documentation as a known issue/limitation/behaviour (not sure where, API doc?).

@KtorZ
Copy link
Member Author

KtorZ commented Mar 18, 2019

Well, it's not that we don't support SIGKILL, we do. But the semantic of SIGKILL doesn't allow us to do any clean-up after receiving it. So, if one kills the launcher, one doesn't kill the sub-processes the launcher has started.

Yet, ideally, we do want to "link" our launcher to the spawned processes at the system level, such that the OS will make sure to terminate the subprocesses when we kill the parent. One way to do that is to have a shared socket between the launcher and its children (which will anyway be the case with the new Haskell node since we'll communicate through a pipe).

@piotr-iohk
Copy link
Contributor

Well, it's not that we don't support SIGKILL, we do. But the semantic of SIGKILL doesn't allow us to do any clean-up after receiving it. So, if one kills the launcher, one doesn't kill the sub-processes the launcher has started.

Well, not very fortunate selection of words on my side but that's what I actually meant by saying don't support ^ ;) i.e. that we don't clean up after kill -9 as of now. So if we wan't to go for this shared socket now (or very soon) to support it - that's cool :). If not at this point (or not in the near future), that's also OK, but then it would be worth to state it somewhere as a known limitation. That's what I meant. :)

@KtorZ KtorZ force-pushed the KtorZ/8/sigterm-handler-tests branch from d916efc to fc8acc0 Compare March 18, 2019 14:42
@KtorZ KtorZ force-pushed the KtorZ/8/sigterm-handler-tests branch from fc8acc0 to 5a74429 Compare March 18, 2019 14:44
@KtorZ KtorZ requested a review from piotr-iohk March 18, 2019 16:26
Copy link

@pppsss40 pppsss40 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@piotr-iohk piotr-iohk merged commit 8b1bcbc into master Mar 18, 2019
@KtorZ KtorZ deleted the KtorZ/8/sigterm-handler-tests branch March 19, 2019 10:10
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.

None yet

3 participants