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

Error while shutting down parallel: unable to terminate some child processes #236

Closed
kforner opened this issue Feb 5, 2020 · 10 comments
Closed

Comments

@kforner
Copy link

@kforner kforner commented Feb 5, 2020

As discussed in #113, there seems to be an interaction between parallel::mclapply and processx related to a signal handler.

The symptom is this message when leaving a R session or entering debug:
Error while shutting down parallel: unable to terminate some child processes

It also comes with a noticeable delay to quit or enter the debug session.

I could not manage to build a reproducible example yet.
And actually I do not use processx directly but callr.

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Here is a reprex:

# Save this into a file and run it from the command line as
# R --no-save -q < <filename>

library(parallel)

# Create cluster, check that it works
cl <- parallel::makeForkCluster(2)
mclapply(1:2, function(x) x)

# Double check that subprocesses are running
ps::ps_children(ps::ps_handle())

# Run a parallel background job
job <- mcparallel(Sys.sleep(1))

# Start processx process, it will overwrite the signal handler
processx::run("true")

# Wait for parallel job to finish
mccollect(job)

# Quit, this will hang a big, and then quit with a message,
# but otherwise a zero (=successful) exit status
gaborcsardi added a commit that referenced this issue Feb 5, 2020
This is now opt in, and the PROCESSX_NOTIFY_OLD_SIGCHLD
env var has to be set (to any value), at the time of
_loading_ of the processx package.

Fixes #236.
@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

@kforner can you pls try #237?

For now this is opt-in and you need to set the PROCESSX_NOTIFY_OLD_SIGCHLD env var to some non-empty value, before processx is loaded. It seems that it is ok to notify parallel's signal handler, even after the parallel cluster has stopped, which is good.

It is surely not OK to unload parallel, though, that's a crash for sure. FYI.

@kforner

This comment has been minimized.

Copy link
Author

@kforner kforner commented Feb 5, 2020

great ! I confirm it exhibits the same behavior I'm getting.

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Wait, so does this PR (#237) fix the issue for you?

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Oh, you mean the reprex works? That's good. Can you pls try the PR? Thanks!

@kforner

This comment has been minimized.

Copy link
Author

@kforner kforner commented Feb 5, 2020

the PR seems to work pretty well!
It fixes your reprex, and also one of my non-minimal use cases.
I will use from now by default and tell you if I see any side-effects.

Thanks a lot !
P.S: Do you plan to merge it soon or should I use this branch ?

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Yes, I'll merge it today, and a new release also coming this week.

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Btw. I want to document this somewhere. Where would you look for documentation like this? Maybe a FAQ would be appropriate?

@kforner

This comment has been minimized.

Copy link
Author

@kforner kforner commented Feb 5, 2020

good question... Yes a FAQ at the r-lib level.
Maybe also mention it in processx/callr/rcmdcheck package synopsis (or main function doc).
Anyway, people will search for the error message and end up on posts as this one.
You could conclude this issue by the link on the FAQ ?

@gaborcsardi

This comment has been minimized.

Copy link
Member

@gaborcsardi gaborcsardi commented Feb 5, 2020

Good idea, but we don't really have an r-lib level FAQ, and it is not clear where that would go. So for now, I just put it in the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.