[v3] mingw: workaround for hangs when sending STDIN
diff mbox series

Message ID pull.553.v3.git.1581962486972.gitgitgadget@gmail.com
State New
Headers show
Series
  • [v3] mingw: workaround for hangs when sending STDIN
Related show

Commit Message

Allo via GitGitGadget Feb. 17, 2020, 6:01 p.m. UTC
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Explanation
-----------
The problem here is flawed `poll()` implementation. When it tries to
see if pipe can be written without blocking, it eventually calls
`NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,
the meaning of quota was misunderstood. The value of quota is reduced
when either some data was written to a pipe, *or* there is a pending
read on the pipe. Therefore, if there is a pending read of size >= then
the pipe's buffer size, poll() will think that pipe is not writable and
will hang forever, usually that means deadlocking both pipe users.

I have studied the problem and found that Windows pipes track two values:
`QuotaUsed` and `BytesInQueue`. The code in `poll()` apparently wants to
know `BytesInQueue` instead of quota. Unfortunately, `BytesInQueue` can
only be requested from read end of the pipe, while `poll()` receives
write end.

The git's implementation of `poll()` was copied from gnulib, which also
contains a flawed implementation up to today.

I also had a look at implementation in cygwin, which is also broken in a
subtle way. It uses this code in `pipe_data_available()`:
	fpli.WriteQuotaAvailable = (fpli.OutboundQuota - fpli.ReadDataAvailable)
However, `ReadDataAvailable` always returns 0 for the write end of the pipe,
turning the code into an obfuscated version of returning pipe's total
buffer size, which I guess will in turn have `poll()` always say that pipe
is writable. The commit that introduced the code doesn't say anything about
this change, so it could be some debugging code that slipped in.

These are the typical sizes used in git:
0x2000 - default read size in `strbuf_read()`
0x1000 - default read size in CRT, used by `strbuf_getwholeline()`
0x2000 - pipe buffer size in compat\mingw.c

As a consequence, as soon as child process uses `strbuf_read()`,
`poll()` in parent process will hang forever, deadlocking both
processes.

This results in two observable behaviors:
1) If parent process begins sending STDIN quickly (and usually that's
   the case), then first `poll()` will succeed and first block will go
   through. MAX_IO_SIZE_DEFAULT is 8MB, so if STDIN exceeds 8MB, then
   it will deadlock.
2) If parent process waits a little bit for any reason (including OS
   scheduler) and child is first to issue `strbuf_read()`, then it will
   deadlock immediately even on small STDINs.

The problem is illustrated by `git stash push`, which will currently
read the entire patch into memory and then send it to `git apply` via
STDIN. If patch exceeds 8MB, git hangs on Windows.

Possible solutions
------------------
1) Somehow obtain `BytesInQueue` instead of `QuotaUsed`
   I did a pretty thorough search and didn't find any ways to obtain
   the value from write end of the pipe.
2) Also give read end of the pipe to `poll()`
   That can be done, but it will probably invite some dirty code,
   because `poll()`
   * can accept multiple pipes at once
   * can accept things that are not pipes
   * is expected to have a well known signature.
3) Make `poll()` always reply "writable" for write end of the pipe
   Afterall it seems that cygwin (accidentally?) does that for years.
   Also, it should be noted that `pump_io_round()` writes 8MB blocks,
   completely ignoring the fact that pipe's buffer size is only 8KB,
   which means that pipe gets clogged many times during that single
   write. This may invite a deadlock, if child's STDERR/STDOUT gets
   clogged while it's trying to deal with 8MB of STDIN. Such deadlocks
   could be defeated with writing less than pipe's buffer size per
   round, and always reading everything from STDOUT/STDERR before
   starting next round. Therefore, making `poll()` always reply
   "writable" shouldn't cause any new issues or block any future
   solutions.
4) Increase the size of the pipe's buffer
   The difference between `BytesInQueue` and `QuotaUsed` is the size
   of pending reads. Therefore, if buffer is bigger than size of reads,
   `poll()` won't hang so easily. However, I found that for example
   `strbuf_read()` will get more and more hungry as it reads large inputs,
   eventually surpassing any reasonable pipe buffer size.

Chosen solution
---------------
Make `poll()` always reply "writable" for write end of the pipe.
Hopefully one day someone will find a way to implement it properly.

Reproduction
------------
printf "%8388608s" X >large_file.txt
git stash push --include-untracked -- large_file.txt

I have decided not to include this as test to avoid slowing down the
test suite. I don't expect the specific problem to come back, and
chances are that `git stash push` will be reworked to avoid sending the
entire patch via STDIN.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
    mingw: git stash push hangs if patch > 8MB
    
    Changes since V2
    ------------------
    Moved test to commit message.
    
    Changes since V1
    ------------------
    Some polishing based on code review in V1
    1) Fixed some spelling in commit message
    2) Reworked test to be more compatible with different shells
    
    ------------------
    Please read the commit message for more information.
    
    The specific problem of `git stash push` exists since `git stash`
    was converted into built-in [1].
    
    On a side note, I think that `git stash push` could be optimized by
    replacing the code that reads entire `git diff-index` into memory
    and then sends it to `git apply`. With large stash, that could mean
    handling a very large patch.
    
    Is it possible to instead directly invoke (without even starting a
    new process) something like `git revert --no-commit -m 1 7091f172` ?
    
    [1] Commit d553f538 ("stash: convert push to builtin" 2019-02-26)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-553%2FSyntevoAlex%2F%230245(git)_poll_hang-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/553

Range-diff vs v2:

 1:  2a1e8f80c5c ! 1:  869d44923a9 mingw: workaround for hangs when sending STDIN
     @@ -88,6 +88,16 @@
          Make `poll()` always reply "writable" for write end of the pipe.
          Hopefully one day someone will find a way to implement it properly.
      
     +    Reproduction
     +    ------------
     +    printf "%8388608s" X >large_file.txt
     +    git stash push --include-untracked -- large_file.txt
     +
     +    I have decided not to include this as test to avoid slowing down the
     +    test suite. I don't expect the specific problem to come back, and
     +    chances are that `git stash push` will be reworked to avoid sending the
     +    entire patch via STDIN.
     +
          Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
      
       diff --git a/compat/poll/poll.c b/compat/poll/poll.c
     @@ -142,27 +152,3 @@
       	    happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);
       	}
             return happened;
     -
     - diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
     - --- a/t/t3903-stash.sh
     - +++ b/t/t3903-stash.sh
     -@@
     - 	git rev-parse --verify refs/stash:A.t
     - '
     - 
     -+test_expect_success 'stash handles large files' '
     -+	x=0123456789abcde\n && # 16
     -+	x=$x$x$x$x$x$x$x$x  && # 128
     -+	x=$x$x$x$x$x$x$x$x  && # 1k
     -+	x=$x$x$x$x$x$x$x$x  && # 8k
     -+	x=$x$x$x$x$x$x$x$x  && # 64k
     -+	x=$x$x$x$x$x$x$x$x  && # 512k
     -+	x=$x$x$x$x$x$x$x$x  && # 4m
     -+	x=$x$x              && # 8m
     -+	echo $x >large_file.txt &&
     -+	unset x             && # release memory
     -+
     -+	git stash push --include-untracked -- large_file.txt
     -+'
     -+
     - test_done


 compat/poll/poll.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)


base-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41

Comments

Junio C Hamano Feb. 18, 2020, 4:39 p.m. UTC | #1
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> Explanation
> -----------
> The problem here is flawed `poll()` implementation. When it tries to
> see if pipe can be written without blocking, it eventually calls
> `NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,
> the meaning of quota was misunderstood. The value of quota is reduced
> when either some data was written to a pipe, *or* there is a pending
> read on the pipe. Therefore, if there is a pending read of size >= then
> the pipe's buffer size, poll() will think that pipe is not writable and
> will hang forever, usually that means deadlocking both pipe users.
> ...
> Chosen solution
> ---------------
> Make `poll()` always reply "writable" for write end of the pipe.
> Hopefully one day someone will find a way to implement it properly.
>
> Reproduction
> ------------
> printf "%8388608s" X >large_file.txt
> git stash push --include-untracked -- large_file.txt
>
> I have decided not to include this as test to avoid slowing down the
> test suite. I don't expect the specific problem to come back, and
> chances are that `git stash push` will be reworked to avoid sending the
> entire patch via STDIN.
>
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---

Thanks for a detailed description.

I notice that we saw no comments from Windows experts for these
three rounds.  Can somebody give an Ack (or nack) on it at least?

I picked obvious "experts" in the output from 

    $ git shortlog --since=1.year --no-merges master compat/ming\* compat/win\*

Thanks.
Johannes Schindelin Feb. 27, 2020, 9:15 p.m. UTC | #2
Hi Junio & Alex,

On Tue, 18 Feb 2020, Junio C Hamano wrote:

> "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> >
> > Explanation
> > -----------
> > The problem here is flawed `poll()` implementation. When it tries to
> > see if pipe can be written without blocking, it eventually calls
> > `NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,
> > the meaning of quota was misunderstood. The value of quota is reduced
> > when either some data was written to a pipe, *or* there is a pending
> > read on the pipe. Therefore, if there is a pending read of size >= then

I usually try to refrain from grammar policing, but in this case, the typo
"then" (instead of "than") threw me.

Other than that, I think the patch is fine. At least it works as
advertised in my hands.

Thanks,
Dscho

> > the pipe's buffer size, poll() will think that pipe is not writable and
> > will hang forever, usually that means deadlocking both pipe users.
> > ...
> > Chosen solution
> > ---------------
> > Make `poll()` always reply "writable" for write end of the pipe.
> > Hopefully one day someone will find a way to implement it properly.
> >
> > Reproduction
> > ------------
> > printf "%8388608s" X >large_file.txt
> > git stash push --include-untracked -- large_file.txt
> >
> > I have decided not to include this as test to avoid slowing down the
> > test suite. I don't expect the specific problem to come back, and
> > chances are that `git stash push` will be reworked to avoid sending the
> > entire patch via STDIN.
> >
> > Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> > ---
>
> Thanks for a detailed description.
>
> I notice that we saw no comments from Windows experts for these
> three rounds.  Can somebody give an Ack (or nack) on it at least?
>
> I picked obvious "experts" in the output from
>
>     $ git shortlog --since=1.year --no-merges master compat/ming\* compat/win\*
>
> Thanks.
>
Junio C Hamano Feb. 27, 2020, 10:59 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I usually try to refrain from grammar policing, but in this case, the typo
> "then" (instead of "than") threw me.
>
> Other than that, I think the patch is fine. At least it works as
> advertised in my hands.

Thanks, both.

Let's mark it for 'next', then.

Patch
diff mbox series

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index 0e95dd493c9..afa6d245846 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -139,22 +139,10 @@  win32_compute_revents (HANDLE h, int *p_sought)
   INPUT_RECORD *irbuffer;
   DWORD avail, nbuffer;
   BOOL bRet;
-  IO_STATUS_BLOCK iosb;
-  FILE_PIPE_LOCAL_INFORMATION fpli;
-  static PNtQueryInformationFile NtQueryInformationFile;
-  static BOOL once_only;
 
   switch (GetFileType (h))
     {
     case FILE_TYPE_PIPE:
-      if (!once_only)
-	{
-	  NtQueryInformationFile = (PNtQueryInformationFile)(void (*)(void))
-	    GetProcAddress (GetModuleHandleW (L"ntdll.dll"),
-			    "NtQueryInformationFile");
-	  once_only = TRUE;
-	}
-
       happened = 0;
       if (PeekNamedPipe (h, NULL, 0, NULL, &avail, NULL) != 0)
 	{
@@ -166,22 +154,9 @@  win32_compute_revents (HANDLE h, int *p_sought)
 
       else
 	{
-	  /* It was the write-end of the pipe.  Check if it is writable.
-	     If NtQueryInformationFile fails, optimistically assume the pipe is
-	     writable.  This could happen on Win9x, where NtQueryInformationFile
-	     is not available, or if we inherit a pipe that doesn't permit
-	     FILE_READ_ATTRIBUTES access on the write end (I think this should
-	     not happen since WinXP SP2; WINE seems fine too).  Otherwise,
-	     ensure that enough space is available for atomic writes.  */
-	  memset (&iosb, 0, sizeof (iosb));
-	  memset (&fpli, 0, sizeof (fpli));
-
-	  if (!NtQueryInformationFile
-	      || NtQueryInformationFile (h, &iosb, &fpli, sizeof (fpli),
-					 FilePipeLocalInformation)
-	      || fpli.WriteQuotaAvailable >= PIPE_BUF
-	      || (fpli.OutboundQuota < PIPE_BUF &&
-		  fpli.WriteQuotaAvailable == fpli.OutboundQuota))
+	  /* It was the write-end of the pipe. Unfortunately there is no
+	     reliable way of knowing if it can be written without blocking.
+	     Just say that it's all good. */
 	    happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);
 	}
       return happened;