Kill with grace #367
Open
Kill with grace #367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lionel-
commented
Apr 27, 2023
| #include <signal.h> | ||
|
|
||
| void R_init_sigtermignore(DllInfo *dll) { | ||
| signal(SIGTERM, SIG_IGN); |
There was a problem hiding this comment.
Since I wrote this branch I've added sigterm handlers to px in r-lib/ps#149. We could use this instead of this client lib for our unit tests by depending on the dev version of ps.
| .grace = handle->cleanup_grace, | ||
| .name = R_NilValue | ||
| }; | ||
| r_with_cleanup_context(c_processx_kill_data, &data); |
There was a problem hiding this comment.
I'd prefer to use the non-jumpy approach implemented in the ps PR, but I went for minimal changes here.
|
|
||
| int *fds = malloc(sizeof(int) * 2); | ||
| if (!fds) R_THROW_SYSTEM_ERROR("Allocating memory when waiting"); | ||
| fds[0] = fds[1] = -1; | ||
| r_call_on_exit(processx__wait_cleanup, fds); | ||
|
|
||
| processx__block_sigchld(); | ||
| sigset_t old; | ||
| processx__block_sigchld_save(&old); |
There was a problem hiding this comment.
We're now restoring the caller's signal mask on exit since this can now be called from C.
Otherwise a SIGPIPE may freeze an R process, like we saw in parallel testthat tests: r-lib/testthat#1819 Here the main proces is killed, but the worker processes are not, and when a worker process tries to send the test results to the main process, a `write()` on its pipe gets a `SIGPIPE. Unclear why, but the `SIGPIPE` sometimes causes a `SIGSEGV`, for which R tries to print the stack, but gets another `SIGSEGV`, and so on.
[ci ckip]
To please R CMD check :/
To see the rest of the printf format string warnings (#379).
Apparently, this only works correctly with HTTP/1.1, and fails with HTTP/2.
[ci skip]
To avoid a message from R6 at install and `load_all()` time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Branched from #366.
Revive the
graceargument in thekill()method. We now first send aSIGTERM, wait for a grace period, and only thenSIGKILLTo avoid a delay of
graceseconds when the process terminates quickly, we poll for termination. This is supported by the same mechanism that is already implemented forwait(). The routine is split into an R-facingprocessx_wait()function and a C-facingc_processx_wait()variant which is called by the kill implementation.The
new()andrun()methods of processes gain acleanup_graceargument that is passed tokill()andkill_tree()on termination.