search

gap-system / gap

Main development repository for GAP - Groups, Algorithms, Programming, a System for Computational Discrete Algebra
https://www.gap-system.org
GNU General Public License v2.0
814 stars 161 forks source link

Improve the way GAP terminates subprocesses to avoid a race condition that could lead to unwanted warnings or even hangs #5804

Closed fingolfin closed 1 month ago

fingolfin commented 2 months ago

This might help with https://github.com/oscar-system/Oscar.jl/issues/4136 where we the singular GAP package is loaded, and when GAP exits, an AtExit function is run which ends up invoking CloseStream and thus CLOSE_PTY_IOSTREAM -- and we end up closing the file descriptor to the child process before the child properly terminated.

It seems logical to me to call kill, waitpid, close in that order, which this PR implements. Before we did kill, close, waitpid.

Note that I have not yet confirmed this actually fixes the issue (and that issue is not always reproducible, and apparently not at all on macOS, just on Linux... Ah well)

fingolfin commented 2 months ago

In some quick tests it seemed to resolve the issue.

ChrisJefferson commented 2 months ago

It wouldn't shock me if this upset something else -- this kind of thing is hard to do. The better option (but this would be a much bigger change) would be to stop singular printing to stderr, because then it can shout all it likes as it is closed, and it won't disturb anyone.

fingolfin commented 2 months ago

The printing is the least concern; but it also sometimes hangs. Presumably because it is trying to interact with its stdin or stdout which is now gone. So silencing stderr here would in a sense make things worse because when it hangs you'd have even less of a clue as to what is wrong.

fingolfin commented 1 month ago

@ChrisJefferson I've been testing this for a week now and it works well.

I don't see how this could cause problems, do you have anything specific in mind, @ChrisJefferson ?

ChrisJefferson commented 1 month ago

I didn't have any concrete plan, just experience that process cleaning up a annoying :) But seeing as it seems to be improving things, lets try merging and see if anything else pops up.