GeezerVison

Technically, there is no need to explicitly close parent pipe ends because they have O_CLOEXEC. One small advantage of doing that is to reduce the number of descriptors before the following dup2, which could increase their number (in a corner case when any of descriptors 0, 1, 2 is closed in the parent process) and potentially hit resource limits.

Technically, there is no need, but _posixsubprocess does that. In case of doubt, I prefer to mimick _posixsubprocess behavior.

    /* Close parent's pipe ends. */
    if (p2cwrite != -1)
        POSIX_CALL(close(p2cwrite));
    if (c2pread != -1)
        POSIX_CALL(close(c2pread));
    if (errread != -1)
        POSIX_CALL(close(errread));
    POSIX_CALL(close(errpipe_read));

If we decide to skip the close(), I would prefer to also modify _posixsubprocess. But here I only want to use posix_spawn() in more cases, so I prefer to not modify "unrelated" changes :-)

In case of premature failure on X.Close() or os.close(X) the remaining pipes/fds will remain "open". Perhaps it makes sense to use contextlib.ExitStack.

Is it a regression caused by my PR? Or a general remark on existing code? My PR just moves code, no?

Yep, it doesn't look like a regression introduced by you (was already there).

At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.

Yeah, makes sense. I bet there are many other places which can benefit from ExitStack in a similar manner.

At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.