Message ID | pull.553.git.1581619239467.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mingw: workaround for hangs when sending STDIN | expand |
/////////////////////////////////////////////////////////////////////////////// // NTDLL declarations typedef struct _FILE_PIPE_LOCAL_INFORMATION { ULONG NamedPipeType; ULONG NamedPipeConfiguration; ULONG MaximumInstances; ULONG CurrentInstances; ULONG InboundQuota; ULONG ReadDataAvailable; ULONG OutboundQuota; ULONG WriteQuotaAvailable; ULONG NamedPipeState; ULONG NamedPipeEnd; } FILE_PIPE_LOCAL_INFORMATION, * PFILE_PIPE_LOCAL_INFORMATION; typedef struct _IO_STATUS_BLOCK { union { DWORD Status; PVOID Pointer; } u; ULONG_PTR Information; } IO_STATUS_BLOCK, * PIO_STATUS_BLOCK; typedef enum _FILE_INFORMATION_CLASS { FilePipeLocalInformation = 24 } FILE_INFORMATION_CLASS, * PFILE_INFORMATION_CLASS; typedef DWORD (WINAPI* PNtQueryInformationFile)(HANDLE, IO_STATUS_BLOCK*, VOID*, ULONG, FILE_INFORMATION_CLASS); /////////////////////////////////////////////////////////////////////////////// ULONG GetPipeAvailWriteBuffer(HANDLE a_PipeHandle) { static PNtQueryInformationFile NtQueryInformationFile = (PNtQueryInformationFile)GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "NtQueryInformationFile"); IO_STATUS_BLOCK statusBlock = {}; FILE_PIPE_LOCAL_INFORMATION pipeInformation = {}; if (0 != NtQueryInformationFile(a_PipeHandle, &statusBlock, &pipeInformation, sizeof(pipeInformation), FilePipeLocalInformation)) assert(0); return pipeInformation.WriteQuotaAvailable; } void ReadPipe(HANDLE a_Pipe, DWORD a_Size) { void* buffer = malloc(a_Size); DWORD bytesDone = 0; assert(ReadFile(a_Pipe, buffer, a_Size, &bytesDone, NULL)); assert(bytesDone == a_Size); free(buffer); } void WritePipe(HANDLE a_Pipe, DWORD a_Size) { void* buffer = malloc(a_Size); DWORD bytesDone = 0; assert(WriteFile(a_Pipe, buffer, a_Size, &bytesDone, NULL)); assert(bytesDone == a_Size); free(buffer); } struct ThreadReadParam { HANDLE Pipe; DWORD Size; }; DWORD WINAPI ThreadReadPipe(void* a_Param) { const ThreadReadParam* param = (const ThreadReadParam*)a_Param; ReadPipe(param->Pipe, param->Size); return 0; } void Test() { HANDLE readPipe = 0; HANDLE writePipe = 0; const DWORD pipeBufferSize = 0x8000; assert(CreatePipe(&readPipe, &writePipe, NULL, pipeBufferSize)); DWORD expectedBufferSize = pipeBufferSize; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); // Test 1: nothing unexpected here. // Write some data to pipe, occupying portion of write buffer. { const DWORD size = 0x1000; WritePipe(writePipe, size); expectedBufferSize -= size; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); } // Test 2: nothing unexpected here. // Read some of written data, releasing portion of write buffer. { const DWORD size = 0x0800; ReadPipe(readPipe, size); expectedBufferSize += size; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); } // Test 3: nothing unexpected here. // Read remaining written data, releasing entire buffer. { const DWORD size = 0x0800; ReadPipe(readPipe, size); expectedBufferSize += size; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); } // Test 4: that's the unexpected part. // Start reading the empty pipe and this reduces the *write* buffer. { ThreadReadParam param; param.Pipe = readPipe; param.Size = 0x8000; HANDLE thread = CreateThread(NULL, 0, ThreadReadPipe, ¶m, 0, NULL); Sleep(1000); expectedBufferSize -= param.Size; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); // Write pipe to release thread WritePipe(writePipe, param.Size); WaitForSingleObject(thread, INFINITE); CloseHandle(thread); expectedBufferSize += param.Size; assert(expectedBufferSize == GetPipeAvailWriteBuffer(writePipe)); } CloseHandle(writePipe); CloseHandle(readPipe); }
On Thu, Feb 13, 2020 at 1:40 PM Alexandr Miloslavskiy via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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 then pipe's buffer size per s/then/than/ > 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 then size of reads, s/then/than/ > `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. > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > +test_expect_success 'stash handles large files' ' > + printf "%1023s\n%.0s" "x" {1..16384} >large_file.txt && > + git stash push --include-untracked -- large_file.txt > +' Use of {1..16384} is not portable across shells. You should be able to achieve something similar by assigning a really large value to a shell variable and then echoing that value to "large_file.txt". Something like: x=0123456789 x=$x$x$x$x$x$x$x$x$x$x x=$x$x$x$x$x$x$x$x$x$x ...and so on... echo $x >large_file.txt && or any other similar construct.
On 13.02.2020 19:56, Eric Sunshine wrote: >> clogged while it's trying to deal with 8MB of STDIN. Such deadlocks >> could be defeated with writing less then pipe's buffer size per > > s/then/than/ > >> of pending reads. Therefore, if buffer is bigger then size of reads, > > s/then/than/ > >> +test_expect_success 'stash handles large files' ' >> + printf "%1023s\n%.0s" "x" {1..16384} >large_file.txt && >> + git stash push --include-untracked -- large_file.txt >> +' > > Use of {1..16384} is not portable across shells. You should be able to > achieve something similar by assigning a really large value to a shell > variable and then echoing that value to "large_file.txt". Something > like: > > x=0123456789 > x=$x$x$x$x$x$x$x$x$x$x > x=$x$x$x$x$x$x$x$x$x$x > ...and so on... > echo $x >large_file.txt && > > or any other similar construct. Thanks for having a look! I will address these in V2 next week.
diff --git a/compat/poll/poll.c b/compat/poll/poll.c index 0e95dd493c..afa6d24584 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; diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index ea56e85e70..8877ba31ff 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1285,4 +1285,9 @@ test_expect_success 'stash handles skip-worktree entries nicely' ' git rev-parse --verify refs/stash:A.t ' +test_expect_success 'stash handles large files' ' + printf "%1023s\n%.0s" "x" {1..16384} >large_file.txt && + git stash push --include-untracked -- large_file.txt +' + test_done