Set FD_CLOEXEC on stdin/out/err pipe fds in start_command #164
Conversation
This makes it possible to run more than one command started using start_command at the same time. When invoked start_command would create a set of pipes that it uses to communicate with the subprocess that was spawned, the git process that invoked start_command would own the file descriptors corresponding to the ends of the pipes pointing at stdout, stdin and stderr respectivelly. Which meant that if another process is spawned via fork + exec it would inherit all of those file descriptors from the git process, file descriptors that whatever process just got spawned by exec has no idea what to do with, which in some cases would cause the process that got spawned first to hang. For example, if git spawns two subprocesses A and B, and tries to shut down process A first, git closes the corresponding file descriptors, but as process B still has those same file descriptors pointing at the same pipes open, process A is still trying to read from those two pipes and not getting an EOF. Once this fix is applied - the process B does not inherit pipe descriptions, because this flag specifies that the file descriptor should be closed when an exec function is invoked. This makes running multiple commands safe. Signed-off-by: Ignas Mikalajunas <ignas@uber.com>
Welcome to GitGitGadgetHi @Ignas, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
|
/allow Ignas |
|
User Ignas is now allowed to use GitGitGadget. |
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Just a few suggestions, in the hopes that you find them helpful.
| close(cmd->err); | ||
| child_process_clear(cmd); | ||
| errno = failed_errno; | ||
| return -1; |
There was a problem hiding this comment.
How about changing the oneline of the commit message to something that explains more the "why?" than the "what?"? I have something in mind like:
Let only the intended child process inherit the stdin/stdout/stderr pipes
... and then explaining that we optionally create pipes for the standard file descriptors, to be able to feed stdin to the child process in the parent process, and consume stdout and stderr. We already close the pipes' ends in the parent process that are intended for the child process. But when spawning another child process, the other end is still open, and will be inherited by the wrong child process.
I would then try to explain that this does not currently pose a big problem in Git, as pretty much all child processes are short-lived. And then I would try to give a concrete example (not "A" and "B", but rather something like our read-object hook in VFS for Git and any other child process such as a hook (is this understanding correct?)).
Maybe you want to adjust the commit message in this way?
I might very well misunderstand the precise problem/solution, but even in that case, I think it is more important to answer the "why?" than the "what?" in the commit message...
|
@Ignas if you still want to |
2 participants
This makes it possible to run more than one command started using start_command
at the same time.
When invoked start_command would create a set of pipes that it uses to
communicate with the subprocess that was spawned, the git process that invoked
start_command would own the file descriptors corresponding to the ends of the
pipes pointing at stdout, stdin and stderr respectivelly. Which meant that if
another process is spawned via fork + exec it would inherit all of those file
descriptors from the git process, file descriptors that whatever process just
got spawned by exec has no idea what to do with, which in some cases would
cause the process that got spawned first to hang.
For example, if git spawns two subprocesses A and B, and tries to shut down
process A first, git closes the corresponding file descriptors, but as process
B still has those same file descriptors pointing at the same pipes open,
process A is still trying to read from those two pipes and not getting an EOF.
Once this fix is applied - the process B does not inherit pipe descriptions,
because this flag specifies that the file descriptor should be closed when an
exec function is invoked. This makes running multiple commands safe.
Signed-off-by: Ignas Mikalajunas ignas@uber.com
Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use submitGit to conveniently send your Pull
Requests commits to our mailing list.
Please read the "guidelines for contributing" linked above!