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

Allow disabling of GAP's SIGCHLD handler #3380

Open
embray opened this issue Mar 26, 2019 · 5 comments
Open

Allow disabling of GAP's SIGCHLD handler #3380

embray opened this issue Mar 26, 2019 · 5 comments
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: libgap things related to libgap
Projects

Comments

@embray
Copy link
Contributor

embray commented Mar 26, 2019

Per discussion toward the end of #3072, for the sake of embedding libgap in other systems, it should be possible for the SIGCHLD handler installed by GAP to be disabled, probably by setting the same flag that was added in #3072.

The current implementation of the SIGCHLD handling is problematic (see ChildStatusChanged, because it calls waitpid(-1, ...) for any zombie child processes that might still be hanging around un-waited-upon. This is done because historically, some GAP code that launched child processes was less consistent about keeping track of their PIDs, and properly wait()-ing on just those PIDs. Hence a blanket waitpid(-1) sweep to clean up any child processes that might be hanging around.

The problem with this from an embedding perspective is that programs which embed libgap may run and manage their own subprocesses which have nothing to do with those started by GAP code, and having GAP call wait() on those processes could interfere with their management by the code that started them.

We should make sure there are no more areas in the core GAP kernel or libraries that leave around zombie child processes. If there are any packages that somehow do this, responsibility of fixing the issue should be pushed on to that package.

Then the only question is whether the handleSignals flag should disable GAP's SIGCHLD handling completely. And if so, as GAP does still (IMO legitimately) use SIGCHLD handle the completion of processes it is tracking, it should be documented how software which embeds libgap can/should integrate either ChildStatusChanged or CheckChildStatusChanged into its own SIGCHLD handling.

@ChrisJefferson
Copy link
Contributor

Just to extend on this, on initial inspection I think the GAP kernel does clean up after itself. Someone else should carefully check that there is no way for childPID in particular to be leaked.

After that the big issue is the IO package. Unfortunately I think the fix there will be large and expensive:

  1. Remember all child PIDs
  2. Loop over that list whenever there is a sigchld
  3. Probably remove the whole add/remove IO signal handler business (maybe leave the functions in).

This will make various things much more expensive, as we'd be changing a single waitpid(-1) for an O(n) pass over all child PIDs. We'd have to benchmark to see how much more expensive and if the cost is reasonable for standard GAP usage.

@jdemeyer
Copy link

To clarify: for the purposes of SageMath, the fact that SIGCHLD is handled is not a problem. The problem is the waitpid(-1, ...) inside the handler.

@embray
Copy link
Contributor Author

embray commented Mar 26, 2019

Yes, but I'm trying to treat this issue more generally than just as it applies to Sage.

@jdemeyer
Copy link

This will make various things much more expensive, as we'd be changing a single waitpid(-1) for an O(n) pass over all child PIDs.

Are there actual use cases involving a large number of child processes that need to be checked frequently? On my (relatively old) Linux laptop, a single waitpid() call takes about 180ns. It's not negligible, but it's not a disaster either. You could do 100000 waitpid() calls per second and hardly notice the difference.

@jdemeyer
Copy link

Cross-reference: gap-packages/io#5

@wilfwilson wilfwilson added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: libgap things related to libgap labels Mar 26, 2019
@fingolfin fingolfin added this to TODO in LibGAP Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: libgap things related to libgap
Projects
LibGAP
  
TODO
Development

No branches or pull requests

4 participants