[PATCH] status: read submodule process output before calling wait()
- From
Jeff King <peff@peff.net>
- Date
- Mar 22, 2015, 07:44 UTC
- Message-ID
- <20150322074455.GA1303@peff.net>
- In-Reply-To
- <CAPx-p4sKD0Nut3E1jnWfPPx4=--ZOxBgiVdt3RRb5tktw31qDg@mail.gmail.com>
On Sat, Mar 21, 2015 at 10:56:54PM -0700, Wincent Colaiuta wrote:
Show 7 quoted lines
> I've never seen this hang before despite frequent use of submodules. > Oddly, I was able to work around the hang by moving the submodule in > two hops (one from Ansible v1.6.10 to v1.7.0, then from v1.7.0 to > v1.8.4). I am not sure if this is specific to the Ansible repo, or > whether the length of the summary is crossing some threshold that > triggers the bug to manifest. If I run the forked commands manually > from an interactive shell, they complete just fine.
It's the length of the summary. The fix is below.
-- >8 -- The status code tries to read the output of "git submodule summary" over a pipe by waiting for the program to finish and then reading its output, like this:
run_command(&sm_summary); len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
Besides being a violation of the run-command API (which makes no promises about the state of the struct after run_command returns), this can easily lead to deadlock. The "submodule status" process may fill up the pipe buffer and block on write(). Meanwhile, the reading side in the parent process is blocked in wait(), waiting for the child to finish.
Instead, we should start the process, read everything it produces, and only then call wait() to finish it off.
Signed-off-by: Jeff King <peff@peff.net> --- I notice that we also don't detect when the sub-command fails. I don't know what we would do in that case (abort the status? print a message?) and it's orthogonal to this issue, so I left it for somebody more clueful in the area to think about.
wt-status.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/wt-status.c b/wt-status.c index 7036fa2..96f0033 100644 --- a/wt-status.c +++ b/wt-status.c @@ -748,9 +748,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt fflush(s->fp); sm_summary.out = -1; - run_command(&sm_summary); - + start_command(&sm_summary); len = strbuf_read(&cmd_stdout, sm_summary.out, 1024); + finish_command(&sm_summary); /* prepend header, only if there's an actual output */ if (len) {
-- 2.3.3.618.ga041503