Message ID | pull.553.v3.git.1581962486972.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] mingw: workaround for hangs when sending STDIN | expand |
"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.
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. >
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.
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;