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

on.exit() callbacks do not run when r() receives SIGINT #99

Closed
wlandau opened this issue Mar 8, 2019 · 11 comments
Closed

on.exit() callbacks do not run when r() receives SIGINT #99

wlandau opened this issue Mar 8, 2019 · 11 comments

Comments

@wlandau
Copy link

wlandau commented Mar 8, 2019

Reprex: mschubert/clustermq#130 (comment)

@gaborcsardi
Copy link
Member

gaborcsardi commented Mar 8, 2019

Processx kills the child process on interrupt.

on.exit() is not a very good solution to clean up grandchild processes, because it will not work if the child process crashes or (as in this case) is killed.

@gaborcsardi
Copy link
Member

It is also not worth sending a SIGINT, because R often ignores that.

@wlandau
Copy link
Author

wlandau commented Mar 8, 2019

Fair enough, thanks @gaborcsardi. I will explore workarounds.

@wlandau wlandau closed this as completed Mar 8, 2019
@gaborcsardi
Copy link
Member

So if your goal is to simply clean up the whole process tree, processx::run() (called by callr::r()) has an option for this (cleanup_tree).

callr does not use it currently, but it should.

@wlandau
Copy link
Author

wlandau commented Mar 8, 2019

Yes, my goal is to clean up the whole process tree. Would you be open to a pull request that adds a formal cleanup_tree argument to r() and friends?

@gaborcsardi
Copy link
Member

I think they should just forward ... to processx::run().

@wlandau
Copy link
Author

wlandau commented Mar 8, 2019

Yes, my goal is to clean up the whole process tree. Would you be open to a pull request that adds a formal cleanup_tree argument to r() and friends?

Hmm... I think I misspoke. For this to improve ropensci/drake#772, I think I would need HPC jobs to be connected to the local process tree through handlers that respect SIGINT.

@gaborcsardi
Copy link
Member

gaborcsardi commented Mar 8, 2019

Well, if you are sure that your R code respects SIGINT (e.g. it does not have compiled code), then you can start an R subprocess in the background with r_bg() or r_process$new() directly, and then send a SIGINT with the $interrupt() or
$signal() method, as needed.

Note that on windows there is no SIGINT, but you can still use $interrupt().

@mschubert
Copy link

It is also not worth sending a SIGINT, because R often ignores that.

@gaborcsardi Shouldn't this be up the the code executed by callr::r()? If it uses external resources that need to be cleaned up, it would install a handler.

However, if callr is sending a SIGKILL, there is no possibility to respond to that.

Would you consider sending SIGINT, add a small delay, and then send SIGTERM/SIGKILL?

@gaborcsardi
Copy link
Member

Shouldn't this be up the the code executed by callr::r()? If it uses external resources that need to be cleaned up, it would install a handler.

Well, not really, because R itself installs a handler. This handler does a clean interrupt, and in non-interactive mode also exits. But in compiled code it ignores interrupts, unless the compiled code explicitly checks for them.

Removing the default handler and installing another one is not sg I would recommend, and AFAIR R reinstalls its handler frequently, anyway, so might not be even possible.

Would you consider sending SIGINT, add a small delay, and then send SIGTERM/SIGKILL?

I suppose I could do that, although this is not trivial, because of pid reuse, and I also don't see how it helps the original issue, because you can't implement reliable cleanup on top of this.

@mschubert
Copy link

Well, not really, because R itself installs a handler.

I was more thinking about a handler via R and less about an C-level signal handler. So for instance:

callr::r({
    tryCatch({ something that mounts a resource }, interrupt=function(e) unmount(resource))
})

As far as I understand, this would clean up its resources upon SIGINT+SIGTERM, but not with only SIGTERM?

I also don't see how it helps the original issue, because you can't implement reliable cleanup on top of this

I'm not sure I understand this. on.exit() will also be called on interrupt, so shouldn't this be usable for cleanup as well? Or was your point that this is not reliable in case it is killed?


Or if sticking with SIGTERM (I assume) only: Are you aware of any possibility to handle SIGTERM from R code?

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

No branches or pull requests

3 participants