| 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