[ re #2944, fix ] Make popen2 be able to not to leak system resources
#3170
Conversation
This file contains 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
This fixes problem of creation the garbage of zombie processes on POSIX systems. This also makes behaviour of `popen2` identical in Windows, namely, all resources are freed only when waiting, giving at the same time an ability to observe the exit code afterwards.
mattpolzin
approved these changes
Dec 19, 2023
|
Looks good, thanks for taking care of this. |
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.
Description
Current implementation of
popen2function introduces in #2944 has slightly different behaviour for POSIX and Windows systems, and, more importantly, it leaves zombie processes on POSIX systems, which, when being run regularly, leads to exhaustiveness of PIDs in a system.Anyway, leaky and unstable behaviour is not to nice, so I propose the simplest solution I came with. I suggest to make behaviour similar in POSIX and WIndows AND requiring a cleanup call
popen2Wait. Roughtly speaking, this is not a much stretch, because for usualpopenwe already have a pair functionpclosewith the similar meaning. We can't have direct "closing" analogue forpopen2, because moment and order of closing of the file handlers depends on a particular use (unlikepopen), so, full cleanup has to be in a series of closings of file handlers and then calling to the newly added functionpopen2Wait.Behaviour of the existing (leaky) Idris code that currently uses existing
popen2function remains unchanged with this PR (being run on POSIX systems). So this PR shouldn't break anything.Alternative solution of the leaking problem was considered (and even mosly implemented in a separate branch). I produces fully detached process which does not produce a zombie in the end, disallowing, however, to get the exit code of this process (at least, easily). But this solution (suddenly) happened to be much more complicated, and, after several attempts to design nicely, I realised that this alternative solution should contain the solution from this PR, so we should first do this before thinking whether we need an ability of detached processes or not.
Pinging @dunhamsteve and @mattpolzin as the original's PR author and main reviewer.
Should this change go in the CHANGELOG?
implementation, I have updated
CHANGELOG.md(and potentially alsoCONTRIBUTORS.md).