{"thread":{"id":"52798","subject":"[PATCH] mingw: workaround for hangs when sending STDIN","startedAt":"2020-02-13T18:40:46Z","lastAt":"2020-02-27T22:59:13Z","messageCount":12,"participants":["Alexandr Miloslavskiy via GitGitGadget","Alexandr Miloslavskiy","Eric Sunshine","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391685","messageId":"pull.553.git.1581619239467.gitgitgadget@gmail.com","threadId":"52798","inReplyTo":null,"subject":"[PATCH] mingw: workaround for hangs when sending STDIN","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-13T18:40:39Z","receivedAt":"2020-02-13T18:40:46Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nExplanation\n-----------\nThe problem here is flawed `poll()` implementation. When it tries to\nsee if pipe can be written without blocking, it eventually calls\n`NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,\nthe meaning of quota was misunderstood. The value of quota is reduced\nwhen either some data was written to a pipe, *or* there is a pending\nread on the pipe. Therefore, if there is a pending read of size >= then\nthe pipe's buffer size, poll() will think that pipe is not writable and\nwill hang forever, usually that means deadlocking both pipe users.\n\nI have studied the problem and found that Windows pipes track two values:\n`QuotaUsed` and `BytesInQueue`. The code in `poll()` apparently wants to\nknow `BytesInQueue` instead of quota. Unfortunately, `BytesInQueue` can\nonly be requested from read end of the pipe, while `poll()` receives\nwrite end.\n\nThe git's implementation of `poll()` was copied from gnulib, which also\ncontains a flawed implementation up to today.\n\nI also had a look at implementation in cygwin, which is also broken in a\nsubtle way. It uses this code in `pipe_data_available()`:\n\tfpli.WriteQuotaAvailable = (fpli.OutboundQuota - fpli.ReadDataAvailable)\nHowever, `ReadDataAvailable` always returns 0 for the write end of the pipe,\nturning the code into an obfuscated version of returning pipe's total\nbuffer size, which I guess will in turn have `poll()` always say that pipe\nis writable. The commit that introduced the code doesn't say anything about\nthis change, so it could be some debugging code that slipped in.\n\nThese are the typical sizes used in git:\n0x2000 - default read size in `strbuf_read()`\n0x1000 - default read size in CRT, used by `strbuf_getwholeline()`\n0x2000 - pipe buffer size in compat\\mingw.c\n\nAs a consequence, as soon as child process uses `strbuf_read()`,\n`poll()` in parent process will hang forever, deadlocking both\nprocesses.\n\nThis results in two observable behaviors:\n1) If parent process begins sending STDIN quickly (and usually that's\n the case), then first `poll()` will succeed and first block will go\n through. MAX_IO_SIZE_DEFAULT is 8MB, so if STDIN exceeds 8MB, then\n it will deadlock.\n2) If parent process waits a little bit for any reason (including OS\n scheduler) and child is first to issue `strbuf_read()`, then it will\n deadlock immediately even on small STDINs.\n\nPossible solutions\n------------------\n1) Somehow obtain `BytesInQueue` instead of `QuotaUsed`\n I did a pretty thorough search and didn't find any ways to obtain\n the value from write end of the pipe.\n2) Also give read end of the pipe to `poll()`\n That can be done, but it will probably invite some dirty code,\n because `poll()`\n * can accept multiple pipes at once\n * can accept things that are not pipes\n * is expected to have a well known signature.\n3) Make `poll()` always reply \"writable\" for write end of the pipe\n Afterall it seems that cygwin (accidentally?) does that for years.\n Also, it should be noted that `pump_io_round()` writes 8MB blocks,\n completely ignoring the fact that pipe's buffer size is only 8KB,\n which means that pipe gets clogged many times during that single\n write. This may invite a deadlock, if child's STDERR/STDOUT gets\n clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n could be defeated with writing less then pipe's buffer size per\n round, and always reading everything from STDOUT/STDERR before\n starting next round. Therefore, making `poll()` always reply\n \"writable\" shouldn't cause any new issues or block any future\n solutions.\n4) Increase the size of the pipe's buffer\n The difference between `BytesInQueue` and `QuotaUsed` is the size\n of pending reads. Therefore, if buffer is bigger then size of reads,\n `poll()` won't hang so easily. However, I found that for example\n `strbuf_read()` will get more and more hungry as it reads large inputs,\n eventually surpassing any reasonable pipe buffer size.\n\nChosen solution\n---------------\nMake `poll()` always reply \"writable\" for write end of the pipe.\nHopefully one day someone will find a way to implement it properly.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n mingw: git stash push hangs if patch > 8MB\n \n Please read the commit message for more information.\n \n The specific problem of `git stash push` exists since `git stash`\n was converted into built-in [1].\n \n On a side note, I think that `git stash push` could be optimized by\n replacing the code that reads entire `git diff-index` into memory\n and then sends it to `git apply`. With large stash, that could mean\n handling a very large patch.\n \n Is it possible to instead directly invoke (without even starting a\n new process) something like `git revert --no-commit -m 1 7091f172` ?\n \n [1] Commit d553f538 (\"stash: convert push to builtin\" 2019-02-26)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-553%2FSyntevoAlex%2F%230245(git)_poll_hang-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/553\n\n compat/poll/poll.c | 31 +++----------------------------\n t/t3903-stash.sh | 5 +++++\n 2 files changed, 8 insertions(+), 28 deletions(-)\n\ndiff --git a/compat/poll/poll.c b/compat/poll/poll.c\nindex 0e95dd493c..afa6d24584 100644\n--- a/compat/poll/poll.c\n+++ b/compat/poll/poll.c\n@@ -139,22 +139,10 @@ win32_compute_revents (HANDLE h, int *p_sought)\n INPUT_RECORD *irbuffer;\n DWORD avail, nbuffer;\n BOOL bRet;\n- IO_STATUS_BLOCK iosb;\n- FILE_PIPE_LOCAL_INFORMATION fpli;\n- static PNtQueryInformationFile NtQueryInformationFile;\n- static BOOL once_only;\n \n switch (GetFileType (h))\n {\n case FILE_TYPE_PIPE:\n- if (!once_only)\n-\t{\n-\t NtQueryInformationFile = (PNtQueryInformationFile)(void (*)(void))\n-\t GetProcAddress (GetModuleHandleW (L\"ntdll.dll\"),\n-\t\t\t \"NtQueryInformationFile\");\n-\t once_only = TRUE;\n-\t}\n-\n happened = 0;\n if (PeekNamedPipe (h, NULL, 0, NULL, &avail, NULL) != 0)\n \t{\n@@ -166,22 +154,9 @@ win32_compute_revents (HANDLE h, int *p_sought)\n \n else\n \t{\n-\t /* It was the write-end of the pipe. Check if it is writable.\n-\t If NtQueryInformationFile fails, optimistically assume the pipe is\n-\t writable. This could happen on Win9x, where NtQueryInformationFile\n-\t is not available, or if we inherit a pipe that doesn't permit\n-\t FILE_READ_ATTRIBUTES access on the write end (I think this should\n-\t not happen since WinXP SP2; WINE seems fine too). Otherwise,\n-\t ensure that enough space is available for atomic writes. */\n-\t memset (&iosb, 0, sizeof (iosb));\n-\t memset (&fpli, 0, sizeof (fpli));\n-\n-\t if (!NtQueryInformationFile\n-\t || NtQueryInformationFile (h, &iosb, &fpli, sizeof (fpli),\n-\t\t\t\t\t FilePipeLocalInformation)\n-\t || fpli.WriteQuotaAvailable >= PIPE_BUF\n-\t || (fpli.OutboundQuota < PIPE_BUF &&\n-\t\t fpli.WriteQuotaAvailable == fpli.OutboundQuota))\n+\t /* It was the write-end of the pipe. Unfortunately there is no\n+\t reliable way of knowing if it can be written without blocking.\n+\t Just say that it's all good. */\n \t happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);\n \t}\n return happened;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex ea56e85e70..8877ba31ff 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1285,4 +1285,9 @@ test_expect_success 'stash handles skip-worktree entries nicely' '\n \tgit rev-parse --verify refs/stash:A.t\n '\n \n+test_expect_success 'stash handles large files' '\n+\tprintf \"%1023s\\n%.0s\" \"x\" {1..16384} >large_file.txt &&\n+\tgit stash push --include-untracked -- large_file.txt\n+'\n+\n test_done\n\nbase-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41\n-- \ngitgitgadget\n"},{"id":"391686","messageId":"86c3fad5-4fa7-7bb0-37cc-7c43e6f2eedc@syntevo.com","threadId":"52798","inReplyTo":"pull.553.git.1581619239467.gitgitgadget@gmail.com","subject":"Test program used to prove quota's behavior","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2020-02-13T18:41:47Z","receivedAt":"2020-02-13T18:49:15Z","isPatch":false,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"///////////////////////////////////////////////////////////////////////////////\n// NTDLL declarations\ntypedef struct _FILE_PIPE_LOCAL_INFORMATION {\n\tULONG NamedPipeType;\n\tULONG NamedPipeConfiguration;\n\tULONG MaximumInstances;\n\tULONG CurrentInstances;\n\tULONG InboundQuota;\n\tULONG ReadDataAvailable;\n\tULONG OutboundQuota;\n\tULONG WriteQuotaAvailable;\n\tULONG NamedPipeState;\n\tULONG NamedPipeEnd;\n} FILE_PIPE_LOCAL_INFORMATION, * PFILE_PIPE_LOCAL_INFORMATION;\n\ntypedef struct _IO_STATUS_BLOCK\n{\n\tunion {\n\t\tDWORD Status;\n\t\tPVOID Pointer;\n\t} u;\n\tULONG_PTR Information;\n} IO_STATUS_BLOCK, * PIO_STATUS_BLOCK;\n\ntypedef enum _FILE_INFORMATION_CLASS {\n\tFilePipeLocalInformation = 24\n} FILE_INFORMATION_CLASS, * PFILE_INFORMATION_CLASS;\n\ntypedef DWORD (WINAPI* PNtQueryInformationFile)(HANDLE, \nIO_STATUS_BLOCK*, VOID*, ULONG, FILE_INFORMATION_CLASS);\n///////////////////////////////////////////////////////////////////////////////\n\nULONG GetPipeAvailWriteBuffer(HANDLE a_PipeHandle)\n{\n\tstatic PNtQueryInformationFile NtQueryInformationFile = \n(PNtQueryInformationFile)GetProcAddress(GetModuleHandleW(L\"ntdll.dll\"), \n\"NtQueryInformationFile\");\n\n\tIO_STATUS_BLOCK statusBlock = {};\n\tFILE_PIPE_LOCAL_INFORMATION pipeInformation = {};\n\tif (0 != NtQueryInformationFile(a_PipeHandle, &statusBlock, \n&pipeInformation, sizeof(pipeInformation), FilePipeLocalInformation))\n\t\tassert(0);\n\n\treturn pipeInformation.WriteQuotaAvailable;\n}\n\nvoid ReadPipe(HANDLE a_Pipe, DWORD a_Size)\n{\n\tvoid* buffer = malloc(a_Size);\n\tDWORD bytesDone = 0;\n\tassert(ReadFile(a_Pipe, buffer, a_Size, &bytesDone, NULL));\n\tassert(bytesDone == a_Size);\n\tfree(buffer);\n}\n\nvoid WritePipe(HANDLE a_Pipe, DWORD a_Size)\n{\n\tvoid* buffer = malloc(a_Size);\n\tDWORD bytesDone = 0;\n\tassert(WriteFile(a_Pipe, buffer, a_Size, &bytesDone, NULL));\n\tassert(bytesDone == a_Size);\n\tfree(buffer);\n}\n\nstruct ThreadReadParam\n{\n\tHANDLE Pipe;\n\tDWORD Size;\n};\n\nDWORD WINAPI ThreadReadPipe(void* a_Param)\n{\n\tconst ThreadReadParam* param = (const ThreadReadParam*)a_Param;\n\tReadPipe(param->Pipe, param->Size);\n\treturn 0;\n}\n\nvoid Test()\n{\n\tHANDLE readPipe = 0;\n\tHANDLE writePipe = 0;\n\tconst DWORD pipeBufferSize = 0x8000;\n\tassert(CreatePipe(&readPipe, &writePipe, NULL, pipeBufferSize));\n\n\tDWORD expectedBufferSize = pipeBufferSize;\n\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\n\t// Test 1: nothing unexpected here.\n\t// Write some data to pipe, occupying portion of write buffer.\n\t{\n\t\tconst DWORD size = 0x1000;\n\t\tWritePipe(writePipe, size);\n\n\t\texpectedBufferSize -= size;\n\t\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\t}\n\n\t// Test 2: nothing unexpected here.\n\t// Read some of written data, releasing portion of write buffer.\n\t{\n\t\tconst DWORD size = 0x0800;\n\t\tReadPipe(readPipe, size);\n\n\t\texpectedBufferSize += size;\n\t\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\t}\n\n\t// Test 3: nothing unexpected here.\n\t// Read remaining written data, releasing entire buffer.\n\t{\n\t\tconst DWORD size = 0x0800;\n\t\tReadPipe(readPipe, size);\n\n\t\texpectedBufferSize += size;\n\t\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\t}\n\n\t// Test 4: that's the unexpected part.\n\t// Start reading the empty pipe and this reduces the *write* buffer.\n\t{\n\t\tThreadReadParam param;\n\t\tparam.Pipe = readPipe;\n\t\tparam.Size = 0x8000;\n\t\t\n\t\tHANDLE thread = CreateThread(NULL, 0, ThreadReadPipe, ¶m, 0, NULL);\n\t\tSleep(1000);\n\t\t\n\t\texpectedBufferSize -= param.Size;\n\t\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\n\t\t// Write pipe to release thread\n\t\tWritePipe(writePipe, param.Size);\n\t\tWaitForSingleObject(thread, INFINITE);\n\t\tCloseHandle(thread);\n\n\t\texpectedBufferSize += param.Size;\n\t\tassert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe));\n\t}\n\n\tCloseHandle(writePipe);\n\tCloseHandle(readPipe);\n}\n"},{"id":"391690","messageId":"CAPig+cQrxKuE=a99zPF7EGUSbye_s5ATEvUkz+EqsTZAfy_CbQ@mail.gmail.com","threadId":"52798","inReplyTo":"pull.553.git.1581619239467.gitgitgadget@gmail.com","subject":"Re: [PATCH] mingw: workaround for hangs when sending STDIN","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-13T18:56:37Z","receivedAt":"2020-02-13T18:56:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 13, 2020 at 1:40 PM Alexandr Miloslavskiy via GitGitGadget\n wrote:\n> 3) Make `poll()` always reply \"writable\" for write end of the pipe\n> Afterall it seems that cygwin (accidentally?) does that for years.\n> Also, it should be noted that `pump_io_round()` writes 8MB blocks,\n> completely ignoring the fact that pipe's buffer size is only 8KB,\n> which means that pipe gets clogged many times during that single\n> write. This may invite a deadlock, if child's STDERR/STDOUT gets\n> clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n> could be defeated with writing less then pipe's buffer size per\n\ns/then/than/\n\n> round, and always reading everything from STDOUT/STDERR before\n> starting next round. Therefore, making `poll()` always reply\n> \"writable\" shouldn't cause any new issues or block any future\n> solutions.\n> 4) Increase the size of the pipe's buffer\n> The difference between `BytesInQueue` and `QuotaUsed` is the size\n> of pending reads. Therefore, if buffer is bigger then size of reads,\n\ns/then/than/\n\n> `poll()` won't hang so easily. However, I found that for example\n> `strbuf_read()` will get more and more hungry as it reads large inputs,\n> eventually surpassing any reasonable pipe buffer size.\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> +test_expect_success 'stash handles large files' '\n> + printf \"%1023s\\n%.0s\" \"x\" {1..16384} >large_file.txt &&\n> + git stash push --include-untracked -- large_file.txt\n> +'\n\nUse of {1..16384} is not portable across shells. You should be able to\nachieve something similar by assigning a really large value to a shell\nvariable and then echoing that value to \"large_file.txt\". Something\nlike:\n\n x=0123456789\n x=$x$x$x$x$x$x$x$x$x$x\n x=$x$x$x$x$x$x$x$x$x$x\n ...and so on...\n echo $x >large_file.txt &&\n\nor any other similar construct.\n"},{"id":"391696","messageId":"b7bb3920-0400-df7f-0464-17d3a3583655@syntevo.com","threadId":"52798","inReplyTo":"CAPig+cQrxKuE=a99zPF7EGUSbye_s5ATEvUkz+EqsTZAfy_CbQ@mail.gmail.com","subject":"Re: [PATCH] mingw: workaround for hangs when sending STDIN","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2020-02-13T19:22:47Z","receivedAt":"2020-02-13T19:22:51Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 13.02.2020 19:56, Eric Sunshine wrote:\n>> clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n>> could be defeated with writing less then pipe's buffer size per\n> \n> s/then/than/\n> \n>> of pending reads. Therefore, if buffer is bigger then size of reads,\n> \n> s/then/than/\n> \n>> +test_expect_success 'stash handles large files' '\n>> + printf \"%1023s\\n%.0s\" \"x\" {1..16384} >large_file.txt &&\n>> + git stash push --include-untracked -- large_file.txt\n>> +'\n> \n> Use of {1..16384} is not portable across shells. You should be able to\n> achieve something similar by assigning a really large value to a shell\n> variable and then echoing that value to \"large_file.txt\". Something\n> like:\n> \n> x=0123456789\n> x=$x$x$x$x$x$x$x$x$x$x\n> x=$x$x$x$x$x$x$x$x$x$x\n> ...and so on...\n> echo $x >large_file.txt &&\n> \n> or any other similar construct.\n\nThanks for having a look! I will address these in V2 next week.\n"},{"id":"391923","messageId":"pull.553.v2.git.1581956750001.gitgitgadget@gmail.com","threadId":"52798","inReplyTo":"pull.553.git.1581619239467.gitgitgadget@gmail.com","subject":"[PATCH v2] mingw: workaround for hangs when sending STDIN","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T16:25:49Z","receivedAt":"2020-02-17T16:25:56Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nExplanation\n-----------\nThe problem here is flawed `poll()` implementation. When it tries to\nsee if pipe can be written without blocking, it eventually calls\n`NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,\nthe meaning of quota was misunderstood. The value of quota is reduced\nwhen either some data was written to a pipe, *or* there is a pending\nread on the pipe. Therefore, if there is a pending read of size >= then\nthe pipe's buffer size, poll() will think that pipe is not writable and\nwill hang forever, usually that means deadlocking both pipe users.\n\nI have studied the problem and found that Windows pipes track two values:\n`QuotaUsed` and `BytesInQueue`. The code in `poll()` apparently wants to\nknow `BytesInQueue` instead of quota. Unfortunately, `BytesInQueue` can\nonly be requested from read end of the pipe, while `poll()` receives\nwrite end.\n\nThe git's implementation of `poll()` was copied from gnulib, which also\ncontains a flawed implementation up to today.\n\nI also had a look at implementation in cygwin, which is also broken in a\nsubtle way. It uses this code in `pipe_data_available()`:\n\tfpli.WriteQuotaAvailable = (fpli.OutboundQuota - fpli.ReadDataAvailable)\nHowever, `ReadDataAvailable` always returns 0 for the write end of the pipe,\nturning the code into an obfuscated version of returning pipe's total\nbuffer size, which I guess will in turn have `poll()` always say that pipe\nis writable. The commit that introduced the code doesn't say anything about\nthis change, so it could be some debugging code that slipped in.\n\nThese are the typical sizes used in git:\n0x2000 - default read size in `strbuf_read()`\n0x1000 - default read size in CRT, used by `strbuf_getwholeline()`\n0x2000 - pipe buffer size in compat\\mingw.c\n\nAs a consequence, as soon as child process uses `strbuf_read()`,\n`poll()` in parent process will hang forever, deadlocking both\nprocesses.\n\nThis results in two observable behaviors:\n1) If parent process begins sending STDIN quickly (and usually that's\n the case), then first `poll()` will succeed and first block will go\n through. MAX_IO_SIZE_DEFAULT is 8MB, so if STDIN exceeds 8MB, then\n it will deadlock.\n2) If parent process waits a little bit for any reason (including OS\n scheduler) and child is first to issue `strbuf_read()`, then it will\n deadlock immediately even on small STDINs.\n\nThe problem is illustrated by `git stash push`, which will currently\nread the entire patch into memory and then send it to `git apply` via\nSTDIN. If patch exceeds 8MB, git hangs on Windows.\n\nPossible solutions\n------------------\n1) Somehow obtain `BytesInQueue` instead of `QuotaUsed`\n I did a pretty thorough search and didn't find any ways to obtain\n the value from write end of the pipe.\n2) Also give read end of the pipe to `poll()`\n That can be done, but it will probably invite some dirty code,\n because `poll()`\n * can accept multiple pipes at once\n * can accept things that are not pipes\n * is expected to have a well known signature.\n3) Make `poll()` always reply \"writable\" for write end of the pipe\n Afterall it seems that cygwin (accidentally?) does that for years.\n Also, it should be noted that `pump_io_round()` writes 8MB blocks,\n completely ignoring the fact that pipe's buffer size is only 8KB,\n which means that pipe gets clogged many times during that single\n write. This may invite a deadlock, if child's STDERR/STDOUT gets\n clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n could be defeated with writing less than pipe's buffer size per\n round, and always reading everything from STDOUT/STDERR before\n starting next round. Therefore, making `poll()` always reply\n \"writable\" shouldn't cause any new issues or block any future\n solutions.\n4) Increase the size of the pipe's buffer\n The difference between `BytesInQueue` and `QuotaUsed` is the size\n of pending reads. Therefore, if buffer is bigger than size of reads,\n `poll()` won't hang so easily. However, I found that for example\n `strbuf_read()` will get more and more hungry as it reads large inputs,\n eventually surpassing any reasonable pipe buffer size.\n\nChosen solution\n---------------\nMake `poll()` always reply \"writable\" for write end of the pipe.\nHopefully one day someone will find a way to implement it properly.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n mingw: git stash push hangs if patch > 8MB\n \n Changes since V1\n ------------------\n Some polishing based on code review in V1\n 1) Fixed some spelling in commit message\n 2) Reworked test to be more compatible with different shells\n \n ------------------\n Please read the commit message for more information.\n \n The specific problem of `git stash push` exists since `git stash`\n was converted into built-in [1].\n \n On a side note, I think that `git stash push` could be optimized by\n replacing the code that reads entire `git diff-index` into memory\n and then sends it to `git apply`. With large stash, that could mean\n handling a very large patch.\n \n Is it possible to instead directly invoke (without even starting a\n new process) something like `git revert --no-commit -m 1 7091f172` ?\n \n [1] Commit d553f538 (\"stash: convert push to builtin\" 2019-02-26)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-553%2FSyntevoAlex%2F%230245(git)_poll_hang-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/553\n\nRange-diff vs v1:\n\n 1: e2cb36c34c2 ! 1: 2a1e8f80c5c mingw: workaround for hangs when sending STDIN\n @@ -49,6 +49,10 @@\n scheduler) and child is first to issue `strbuf_read()`, then it will\n deadlock immediately even on small STDINs.\n \n + The problem is illustrated by `git stash push`, which will currently\n + read the entire patch into memory and then send it to `git apply` via\n + STDIN. If patch exceeds 8MB, git hangs on Windows.\n +\n Possible solutions\n ------------------\n 1) Somehow obtain `BytesInQueue` instead of `QuotaUsed`\n @@ -67,14 +71,14 @@\n which means that pipe gets clogged many times during that single\n write. This may invite a deadlock, if child's STDERR/STDOUT gets\n clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n - could be defeated with writing less then pipe's buffer size per\n + could be defeated with writing less than pipe's buffer size per\n round, and always reading everything from STDOUT/STDERR before\n starting next round. Therefore, making `poll()` always reply\n \"writable\" shouldn't cause any new issues or block any future\n solutions.\n 4) Increase the size of the pipe's buffer\n The difference between `BytesInQueue` and `QuotaUsed` is the size\n - of pending reads. Therefore, if buffer is bigger then size of reads,\n + of pending reads. Therefore, if buffer is bigger than size of reads,\n `poll()` won't hang so easily. However, I found that for example\n `strbuf_read()` will get more and more hungry as it reads large inputs,\n eventually surpassing any reasonable pipe buffer size.\n @@ -147,7 +151,17 @@\n '\n \n +test_expect_success 'stash handles large files' '\n -+\tprintf \"%1023s\\n%.0s\" \"x\" {1..16384} >large_file.txt &&\n ++\tx=0123456789abcde\\n && # 16\n ++\tx=$x$x$x$x$x$x$x$x && # 128\n ++\tx=$x$x$x$x$x$x$x$x && # 1k\n ++\tx=$x$x$x$x$x$x$x$x && # 8k\n ++\tx=$x$x$x$x$x$x$x$x && # 64k\n ++\tx=$x$x$x$x$x$x$x$x && # 512k\n ++\tx=$x$x$x$x$x$x$x$x && # 4m\n ++\tx=$x$x && # 8m\n ++\techo $x >large_file.txt &&\n ++\tunset x && # release memory\n ++\n +\tgit stash push --include-untracked -- large_file.txt\n +'\n +\n\n\n compat/poll/poll.c | 31 +++----------------------------\n t/t3903-stash.sh | 15 +++++++++++++++\n 2 files changed, 18 insertions(+), 28 deletions(-)\n\ndiff --git a/compat/poll/poll.c b/compat/poll/poll.c\nindex 0e95dd493c9..afa6d245846 100644\n--- a/compat/poll/poll.c\n+++ b/compat/poll/poll.c\n@@ -139,22 +139,10 @@ win32_compute_revents (HANDLE h, int *p_sought)\n INPUT_RECORD *irbuffer;\n DWORD avail, nbuffer;\n BOOL bRet;\n- IO_STATUS_BLOCK iosb;\n- FILE_PIPE_LOCAL_INFORMATION fpli;\n- static PNtQueryInformationFile NtQueryInformationFile;\n- static BOOL once_only;\n \n switch (GetFileType (h))\n {\n case FILE_TYPE_PIPE:\n- if (!once_only)\n-\t{\n-\t NtQueryInformationFile = (PNtQueryInformationFile)(void (*)(void))\n-\t GetProcAddress (GetModuleHandleW (L\"ntdll.dll\"),\n-\t\t\t \"NtQueryInformationFile\");\n-\t once_only = TRUE;\n-\t}\n-\n happened = 0;\n if (PeekNamedPipe (h, NULL, 0, NULL, &avail, NULL) != 0)\n \t{\n@@ -166,22 +154,9 @@ win32_compute_revents (HANDLE h, int *p_sought)\n \n else\n \t{\n-\t /* It was the write-end of the pipe. Check if it is writable.\n-\t If NtQueryInformationFile fails, optimistically assume the pipe is\n-\t writable. This could happen on Win9x, where NtQueryInformationFile\n-\t is not available, or if we inherit a pipe that doesn't permit\n-\t FILE_READ_ATTRIBUTES access on the write end (I think this should\n-\t not happen since WinXP SP2; WINE seems fine too). Otherwise,\n-\t ensure that enough space is available for atomic writes. */\n-\t memset (&iosb, 0, sizeof (iosb));\n-\t memset (&fpli, 0, sizeof (fpli));\n-\n-\t if (!NtQueryInformationFile\n-\t || NtQueryInformationFile (h, &iosb, &fpli, sizeof (fpli),\n-\t\t\t\t\t FilePipeLocalInformation)\n-\t || fpli.WriteQuotaAvailable >= PIPE_BUF\n-\t || (fpli.OutboundQuota < PIPE_BUF &&\n-\t\t fpli.WriteQuotaAvailable == fpli.OutboundQuota))\n+\t /* It was the write-end of the pipe. Unfortunately there is no\n+\t reliable way of knowing if it can be written without blocking.\n+\t Just say that it's all good. */\n \t happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);\n \t}\n return happened;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex ea56e85e70d..ed23cd6a7f3 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1285,4 +1285,19 @@ test_expect_success 'stash handles skip-worktree entries nicely' '\n \tgit rev-parse --verify refs/stash:A.t\n '\n \n+test_expect_success 'stash handles large files' '\n+\tx=0123456789abcde\\n && # 16\n+\tx=$x$x$x$x$x$x$x$x && # 128\n+\tx=$x$x$x$x$x$x$x$x && # 1k\n+\tx=$x$x$x$x$x$x$x$x && # 8k\n+\tx=$x$x$x$x$x$x$x$x && # 64k\n+\tx=$x$x$x$x$x$x$x$x && # 512k\n+\tx=$x$x$x$x$x$x$x$x && # 4m\n+\tx=$x$x && # 8m\n+\techo $x >large_file.txt &&\n+\tunset x && # release memory\n+\n+\tgit stash push --include-untracked -- large_file.txt\n+'\n+\n test_done\n\nbase-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41\n-- \ngitgitgadget\n"},{"id":"391927","messageId":"CAPig+cQWMvBi4vkAFMjV7LWjKJudja08ZqVMNfLfALxbBfpzXg@mail.gmail.com","threadId":"52798","inReplyTo":"pull.553.v2.git.1581956750001.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] mingw: workaround for hangs when sending STDIN","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-17T17:24:01Z","receivedAt":"2020-02-17T17:24:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 17, 2020 at 11:26 AM Alexandr Miloslavskiy via\nGitGitGadget wrote:\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> +test_expect_success 'stash handles large files' '\n> + x=0123456789abcde\\n && # 16\n\nDid you intend for the \\n in this assignment to be a literal newline?\nEvery shell with which I tested treats it instead as an escaped 'n'.\n\n> + x=$x$x$x$x$x$x$x$x && # 128\n> + x=$x$x$x$x$x$x$x$x && # 1k\n> + x=$x$x$x$x$x$x$x$x && # 8k\n> + x=$x$x$x$x$x$x$x$x && # 64k\n> + x=$x$x$x$x$x$x$x$x && # 512k\n> + x=$x$x$x$x$x$x$x$x && # 4m\n> + x=$x$x && # 8m\n> + echo $x >large_file.txt &&\n> + unset x && # release memory\n\nBy the way, are the embedded newlines actually important to the test\nitself, or are they just for human consumption if the test fails? I\nask because I was curious about how other tests create large files,\nand found that a mechanism similar to your original (but without the\npitfalls) has been used. For instance, t1050-large.sh uses:\n\n printf \"%2000000s\" X >large1 &&\n\nwhich is plenty portable and (presumably) doesn't have such demanding\nmemory consumption.\n"},{"id":"391940","messageId":"xmqqr1ytksh9.fsf@gitster-ct.c.googlers.com","threadId":"52798","inReplyTo":"CAPig+cQWMvBi4vkAFMjV7LWjKJudja08ZqVMNfLfALxbBfpzXg@mail.gmail.com","subject":"Re: [PATCH v2] mingw: workaround for hangs when sending STDIN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-17T17:56:34Z","receivedAt":"2020-02-17T17:56:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ... For instance, t1050-large.sh uses:\n>\n> printf \"%2000000s\" X >large1 &&\n>\n> which is plenty portable and (presumably) doesn't have such demanding\n> memory consumption.\n\nYes, I had the exact same reaction to echoing large string with\nliteral backslash-en in it ;-) Thanks for reviewing and teaching.\n\n"},{"id":"391942","messageId":"pull.553.v3.git.1581962486972.gitgitgadget@gmail.com","threadId":"52798","inReplyTo":"pull.553.v2.git.1581956750001.gitgitgadget@gmail.com","subject":"[PATCH v3] mingw: workaround for hangs when sending STDIN","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T18:01:26Z","receivedAt":"2020-02-17T18:01:33Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nExplanation\n-----------\nThe problem here is flawed `poll()` implementation. When it tries to\nsee if pipe can be written without blocking, it eventually calls\n`NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,\nthe meaning of quota was misunderstood. The value of quota is reduced\nwhen either some data was written to a pipe, *or* there is a pending\nread on the pipe. Therefore, if there is a pending read of size >= then\nthe pipe's buffer size, poll() will think that pipe is not writable and\nwill hang forever, usually that means deadlocking both pipe users.\n\nI have studied the problem and found that Windows pipes track two values:\n`QuotaUsed` and `BytesInQueue`. The code in `poll()` apparently wants to\nknow `BytesInQueue` instead of quota. Unfortunately, `BytesInQueue` can\nonly be requested from read end of the pipe, while `poll()` receives\nwrite end.\n\nThe git's implementation of `poll()` was copied from gnulib, which also\ncontains a flawed implementation up to today.\n\nI also had a look at implementation in cygwin, which is also broken in a\nsubtle way. It uses this code in `pipe_data_available()`:\n\tfpli.WriteQuotaAvailable = (fpli.OutboundQuota - fpli.ReadDataAvailable)\nHowever, `ReadDataAvailable` always returns 0 for the write end of the pipe,\nturning the code into an obfuscated version of returning pipe's total\nbuffer size, which I guess will in turn have `poll()` always say that pipe\nis writable. The commit that introduced the code doesn't say anything about\nthis change, so it could be some debugging code that slipped in.\n\nThese are the typical sizes used in git:\n0x2000 - default read size in `strbuf_read()`\n0x1000 - default read size in CRT, used by `strbuf_getwholeline()`\n0x2000 - pipe buffer size in compat\\mingw.c\n\nAs a consequence, as soon as child process uses `strbuf_read()`,\n`poll()` in parent process will hang forever, deadlocking both\nprocesses.\n\nThis results in two observable behaviors:\n1) If parent process begins sending STDIN quickly (and usually that's\n the case), then first `poll()` will succeed and first block will go\n through. MAX_IO_SIZE_DEFAULT is 8MB, so if STDIN exceeds 8MB, then\n it will deadlock.\n2) If parent process waits a little bit for any reason (including OS\n scheduler) and child is first to issue `strbuf_read()`, then it will\n deadlock immediately even on small STDINs.\n\nThe problem is illustrated by `git stash push`, which will currently\nread the entire patch into memory and then send it to `git apply` via\nSTDIN. If patch exceeds 8MB, git hangs on Windows.\n\nPossible solutions\n------------------\n1) Somehow obtain `BytesInQueue` instead of `QuotaUsed`\n I did a pretty thorough search and didn't find any ways to obtain\n the value from write end of the pipe.\n2) Also give read end of the pipe to `poll()`\n That can be done, but it will probably invite some dirty code,\n because `poll()`\n * can accept multiple pipes at once\n * can accept things that are not pipes\n * is expected to have a well known signature.\n3) Make `poll()` always reply \"writable\" for write end of the pipe\n Afterall it seems that cygwin (accidentally?) does that for years.\n Also, it should be noted that `pump_io_round()` writes 8MB blocks,\n completely ignoring the fact that pipe's buffer size is only 8KB,\n which means that pipe gets clogged many times during that single\n write. This may invite a deadlock, if child's STDERR/STDOUT gets\n clogged while it's trying to deal with 8MB of STDIN. Such deadlocks\n could be defeated with writing less than pipe's buffer size per\n round, and always reading everything from STDOUT/STDERR before\n starting next round. Therefore, making `poll()` always reply\n \"writable\" shouldn't cause any new issues or block any future\n solutions.\n4) Increase the size of the pipe's buffer\n The difference between `BytesInQueue` and `QuotaUsed` is the size\n of pending reads. Therefore, if buffer is bigger than size of reads,\n `poll()` won't hang so easily. However, I found that for example\n `strbuf_read()` will get more and more hungry as it reads large inputs,\n eventually surpassing any reasonable pipe buffer size.\n\nChosen solution\n---------------\nMake `poll()` always reply \"writable\" for write end of the pipe.\nHopefully one day someone will find a way to implement it properly.\n\nReproduction\n------------\nprintf \"%8388608s\" X >large_file.txt\ngit stash push --include-untracked -- large_file.txt\n\nI have decided not to include this as test to avoid slowing down the\ntest suite. I don't expect the specific problem to come back, and\nchances are that `git stash push` will be reworked to avoid sending the\nentire patch via STDIN.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n mingw: git stash push hangs if patch > 8MB\n \n Changes since V2\n ------------------\n Moved test to commit message.\n \n Changes since V1\n ------------------\n Some polishing based on code review in V1\n 1) Fixed some spelling in commit message\n 2) Reworked test to be more compatible with different shells\n \n ------------------\n Please read the commit message for more information.\n \n The specific problem of `git stash push` exists since `git stash`\n was converted into built-in [1].\n \n On a side note, I think that `git stash push` could be optimized by\n replacing the code that reads entire `git diff-index` into memory\n and then sends it to `git apply`. With large stash, that could mean\n handling a very large patch.\n \n Is it possible to instead directly invoke (without even starting a\n new process) something like `git revert --no-commit -m 1 7091f172` ?\n \n [1] Commit d553f538 (\"stash: convert push to builtin\" 2019-02-26)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-553%2FSyntevoAlex%2F%230245(git)_poll_hang-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/553\n\nRange-diff vs v2:\n\n 1: 2a1e8f80c5c ! 1: 869d44923a9 mingw: workaround for hangs when sending STDIN\n @@ -88,6 +88,16 @@\n Make `poll()` always reply \"writable\" for write end of the pipe.\n Hopefully one day someone will find a way to implement it properly.\n \n + Reproduction\n + ------------\n + printf \"%8388608s\" X >large_file.txt\n + git stash push --include-untracked -- large_file.txt\n +\n + I have decided not to include this as test to avoid slowing down the\n + test suite. I don't expect the specific problem to come back, and\n + chances are that `git stash push` will be reworked to avoid sending the\n + entire patch via STDIN.\n +\n Signed-off-by: Alexandr Miloslavskiy \n \n diff --git a/compat/poll/poll.c b/compat/poll/poll.c\n @@ -142,27 +152,3 @@\n \t happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);\n \t}\n return happened;\n -\n - diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n - --- a/t/t3903-stash.sh\n - +++ b/t/t3903-stash.sh\n -@@\n - \tgit rev-parse --verify refs/stash:A.t\n - '\n - \n -+test_expect_success 'stash handles large files' '\n -+\tx=0123456789abcde\\n && # 16\n -+\tx=$x$x$x$x$x$x$x$x && # 128\n -+\tx=$x$x$x$x$x$x$x$x && # 1k\n -+\tx=$x$x$x$x$x$x$x$x && # 8k\n -+\tx=$x$x$x$x$x$x$x$x && # 64k\n -+\tx=$x$x$x$x$x$x$x$x && # 512k\n -+\tx=$x$x$x$x$x$x$x$x && # 4m\n -+\tx=$x$x && # 8m\n -+\techo $x >large_file.txt &&\n -+\tunset x && # release memory\n -+\n -+\tgit stash push --include-untracked -- large_file.txt\n -+'\n -+\n - test_done\n\n\n compat/poll/poll.c | 31 +++----------------------------\n 1 file changed, 3 insertions(+), 28 deletions(-)\n\ndiff --git a/compat/poll/poll.c b/compat/poll/poll.c\nindex 0e95dd493c9..afa6d245846 100644\n--- a/compat/poll/poll.c\n+++ b/compat/poll/poll.c\n@@ -139,22 +139,10 @@ win32_compute_revents (HANDLE h, int *p_sought)\n INPUT_RECORD *irbuffer;\n DWORD avail, nbuffer;\n BOOL bRet;\n- IO_STATUS_BLOCK iosb;\n- FILE_PIPE_LOCAL_INFORMATION fpli;\n- static PNtQueryInformationFile NtQueryInformationFile;\n- static BOOL once_only;\n \n switch (GetFileType (h))\n {\n case FILE_TYPE_PIPE:\n- if (!once_only)\n-\t{\n-\t NtQueryInformationFile = (PNtQueryInformationFile)(void (*)(void))\n-\t GetProcAddress (GetModuleHandleW (L\"ntdll.dll\"),\n-\t\t\t \"NtQueryInformationFile\");\n-\t once_only = TRUE;\n-\t}\n-\n happened = 0;\n if (PeekNamedPipe (h, NULL, 0, NULL, &avail, NULL) != 0)\n \t{\n@@ -166,22 +154,9 @@ win32_compute_revents (HANDLE h, int *p_sought)\n \n else\n \t{\n-\t /* It was the write-end of the pipe. Check if it is writable.\n-\t If NtQueryInformationFile fails, optimistically assume the pipe is\n-\t writable. This could happen on Win9x, where NtQueryInformationFile\n-\t is not available, or if we inherit a pipe that doesn't permit\n-\t FILE_READ_ATTRIBUTES access on the write end (I think this should\n-\t not happen since WinXP SP2; WINE seems fine too). Otherwise,\n-\t ensure that enough space is available for atomic writes. */\n-\t memset (&iosb, 0, sizeof (iosb));\n-\t memset (&fpli, 0, sizeof (fpli));\n-\n-\t if (!NtQueryInformationFile\n-\t || NtQueryInformationFile (h, &iosb, &fpli, sizeof (fpli),\n-\t\t\t\t\t FilePipeLocalInformation)\n-\t || fpli.WriteQuotaAvailable >= PIPE_BUF\n-\t || (fpli.OutboundQuota < PIPE_BUF &&\n-\t\t fpli.WriteQuotaAvailable == fpli.OutboundQuota))\n+\t /* It was the write-end of the pipe. Unfortunately there is no\n+\t reliable way of knowing if it can be written without blocking.\n+\t Just say that it's all good. */\n \t happened |= *p_sought & (POLLOUT | POLLWRNORM | POLLWRBAND);\n \t}\n return happened;\n\nbase-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41\n-- \ngitgitgadget\n"},{"id":"391943","messageId":"11825e4b-c92c-d5ad-9a6f-5fa89d48862c@syntevo.com","threadId":"52798","inReplyTo":"CAPig+cQWMvBi4vkAFMjV7LWjKJudja08ZqVMNfLfALxbBfpzXg@mail.gmail.com","subject":"Re: [PATCH v2] mingw: workaround for hangs when sending STDIN","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2020-02-17T18:01:57Z","receivedAt":"2020-02-17T18:02:02Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 17.02.2020 18:24, Eric Sunshine wrote:\n>> + x=0123456789abcde\\n && # 16\n> \n> Did you intend for the \\n in this assignment to be a literal newline?\n> Every shell with which I tested treats it instead as an escaped 'n'.\n\nI'm such a novice shell script writer :(\nYes, I intended a newline.\n\n> By the way, are the embedded newlines actually important to the test\n> itself, or are they just for human consumption if the test fails?I\n> ask because I was curious about how other tests create large files,\n> and found that a mechanism similar to your original (but without the\n> pitfalls) has been used. For instance, t1050-large.sh uses:\n> \n> printf \"%2000000s\" X >large1 &&\n> \n> which is plenty portable and (presumably) doesn't have such demanding\n> memory consumption.\n\nThey are not important to the test; the test only needs to internally \nhave a 8+ mb patch.\n\nThis only comes from my feeling that super-large lines could cause other \nunexpected things, such as hitting various completely reasonable limits \nand/or causing unwanted slowdowns. Frankly, I didn't test.\n\nFrankly, I already had concerns about adding the test. Now I have \nre-evaluated things and finally decided to move the test into commit \nmessage instead. With it, all compatibility etc questions are resolved.\n"},{"id":"391992","messageId":"xmqqy2szkfxr.fsf@gitster-ct.c.googlers.com","threadId":"52798","inReplyTo":"pull.553.v3.git.1581962486972.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] mingw: workaround for hangs when sending STDIN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-18T16:39:44Z","receivedAt":"2020-02-18T16:39:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alexandr Miloslavskiy via GitGitGadget\" \nwrites:\n\n> From: Alexandr Miloslavskiy \n>\n> Explanation\n> -----------\n> The problem here is flawed `poll()` implementation. When it tries to\n> see if pipe can be written without blocking, it eventually calls\n> `NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,\n> the meaning of quota was misunderstood. The value of quota is reduced\n> when either some data was written to a pipe, *or* there is a pending\n> read on the pipe. Therefore, if there is a pending read of size >= then\n> the pipe's buffer size, poll() will think that pipe is not writable and\n> will hang forever, usually that means deadlocking both pipe users.\n> ...\n> Chosen solution\n> ---------------\n> Make `poll()` always reply \"writable\" for write end of the pipe.\n> Hopefully one day someone will find a way to implement it properly.\n>\n> Reproduction\n> ------------\n> printf \"%8388608s\" X >large_file.txt\n> git stash push --include-untracked -- large_file.txt\n>\n> I have decided not to include this as test to avoid slowing down the\n> test suite. I don't expect the specific problem to come back, and\n> chances are that `git stash push` will be reworked to avoid sending the\n> entire patch via STDIN.\n>\n> Signed-off-by: Alexandr Miloslavskiy \n> ---\n\nThanks for a detailed description.\n\nI notice that we saw no comments from Windows experts for these\nthree rounds. Can somebody give an Ack (or nack) on it at least?\n\nI picked obvious \"experts\" in the output from \n\n $ git shortlog --since=1.year --no-merges master compat/ming\\* compat/win\\*\n\nThanks.\n"},{"id":"392641","messageId":"nycvar.QRO.7.76.6.2002272215210.46@tvgsbejvaqbjf.bet","threadId":"52798","inReplyTo":"xmqqy2szkfxr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] mingw: workaround for hangs when sending STDIN","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-27T21:15:36Z","receivedAt":"2020-02-27T21:15:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Alex,\n\nOn Tue, 18 Feb 2020, Junio C Hamano wrote:\n\n> \"Alexandr Miloslavskiy via GitGitGadget\" \n> writes:\n>\n> > From: Alexandr Miloslavskiy \n> >\n> > Explanation\n> > -----------\n> > The problem here is flawed `poll()` implementation. When it tries to\n> > see if pipe can be written without blocking, it eventually calls\n> > `NtQueryInformationFile()` and tests `WriteQuotaAvailable`. However,\n> > the meaning of quota was misunderstood. The value of quota is reduced\n> > when either some data was written to a pipe, *or* there is a pending\n> > read on the pipe. Therefore, if there is a pending read of size >= then\n\nI usually try to refrain from grammar policing, but in this case, the typo\n\"then\" (instead of \"than\") threw me.\n\nOther than that, I think the patch is fine. At least it works as\nadvertised in my hands.\n\nThanks,\nDscho\n\n> > the pipe's buffer size, poll() will think that pipe is not writable and\n> > will hang forever, usually that means deadlocking both pipe users.\n> > ...\n> > Chosen solution\n> > ---------------\n> > Make `poll()` always reply \"writable\" for write end of the pipe.\n> > Hopefully one day someone will find a way to implement it properly.\n> >\n> > Reproduction\n> > ------------\n> > printf \"%8388608s\" X >large_file.txt\n> > git stash push --include-untracked -- large_file.txt\n> >\n> > I have decided not to include this as test to avoid slowing down the\n> > test suite. I don't expect the specific problem to come back, and\n> > chances are that `git stash push` will be reworked to avoid sending the\n> > entire patch via STDIN.\n> >\n> > Signed-off-by: Alexandr Miloslavskiy \n> > ---\n>\n> Thanks for a detailed description.\n>\n> I notice that we saw no comments from Windows experts for these\n> three rounds. Can somebody give an Ack (or nack) on it at least?\n>\n> I picked obvious \"experts\" in the output from\n>\n> $ git shortlog --since=1.year --no-merges master compat/ming\\* compat/win\\*\n>\n> Thanks.\n>\n"},{"id":"392646","messageId":"xmqqy2sneix3.fsf@gitster-ct.c.googlers.com","threadId":"52798","inReplyTo":"nycvar.QRO.7.76.6.2002272215210.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] mingw: workaround for hangs when sending STDIN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-27T22:59:04Z","receivedAt":"2020-02-27T22:59:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I usually try to refrain from grammar policing, but in this case, the typo\n> \"then\" (instead of \"than\") threw me.\n>\n> Other than that, I think the patch is fine. At least it works as\n> advertised in my hands.\n\nThanks, both.\n\nLet's mark it for 'next', then.\n"}]}