[PATCH] mingw: workaround for hangs when sending STDIN

Alexandr Miloslavskiy via GitGitGadget

author

2020-02-13 18:40 UTC #0 lore raw

PATCH 1/1 mingw: workaround for hangs when sending STDIN

From: Alexandr Miloslavskiy <[email protected]>

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.

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 then 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 then 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.

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
mingw: git stash push hangs if patch > 8MB

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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/553

 compat/poll/poll.c | 31 +++----------------------------
 t/t3903-stash.sh   |  5 +++++
 2 files changed, 8 insertions(+), 28 deletions(-)

compat/poll/poll.c +3 ~~-28~~

index 0e95dd493c..afa6d24584 100644
@@ -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;

t/t3903-stash.sh +5 -0

index ea56e85e70..8877ba31ff 100755
@@ -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

base-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41

Signature · 1 line

gitgitgadget

Alexandr Miloslavskiy

2020-02-13 18:49 UTC #1 lore raw

Test program used to prove quota's behavior

/////////////////////////////////////////////////////////////////////////////// // 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, &param, 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);
}

Eric Sunshine

2020-02-13 18:56 UTC #2 lore raw

In reply to Alexandr Miloslavskiy via GitGitGadget

Quoted 8 lines · Alexandr Miloslavskiy via GitGitGadget

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/

Quoted 7 lines

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/

t/t3903-stash.sh test_expect_success 'stash handles skip-worktree entries nicely' ' · line 1291

Quoted 8 lines

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.
t/t3903-stash.sh +4 -0
+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.

Alexandr Miloslavskiy

2020-02-13 19:22 UTC #3 lore raw

Quoted 19 lines · Eric Sunshine

Quoted 2 lines
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/
Quoted 1 line
of pending reads. Therefore, if buffer is bigger then size of reads,
s/then/than/
t/t3903-stash.sh test_expect_success 'stash handles skip-worktree entries nicely' ' · line 1291
Quoted 4 lines
t/t3903-stash.sh +4 -0
+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:

Quoted 7 lines

     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.

Alexandr Miloslavskiy via GitGitGadget

author

2020-02-17 16:25 UTC #4 lore raw

PATCH v2 1/1 mingw: workaround for hangs when sending STDIN

In reply to Alexandr Miloslavskiy via GitGitGadget

From: Alexandr Miloslavskiy <[email protected]>

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.

Signed-off-by Alexandr Miloslavskiy <[email protected]>

---
mingw: git stash push hangs if patch > 8MB

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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-553/SyntevoAlex/#0245(git)_poll_hang-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/553

Range-diff vs v1:

1: e2cb36c34c2 ! 1: 2a1e8f80c5c mingw: workaround for hangs when sending STDIN
@@ -49,6 +49,10 @@
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
@@ -67,14 +71,14 @@
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
+ 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 then size of reads,
+ 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.
@@ -147,7 +151,17 @@
'

+test_expect_success 'stash handles large files' '
-+ printf "%1023s\n%.0s" "x" {1..16384} >large_file.txt &&
++ 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
+'
+

 compat/poll/poll.c | 31 +++----------------------------
 t/t3903-stash.sh   | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 28 deletions(-)

compat/poll/poll.c +3 ~~-28~~

index 0e95dd493c9..afa6d245846 100644
@@ -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;

t/t3903-stash.sh +15 -0

index ea56e85e70d..ed23cd6a7f3 100755
@@ -1285,4 +1285,19 @@ test_expect_success 'stash handles skip-worktree entries nicely' '
 	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

base-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41

Signature · 1 line

gitgitgadget

Eric Sunshine

2020-02-17 17:24 UTC #5 lore raw

t/t3903-stash.sh test_expect_success 'stash handles skip-worktree entries nicely' ' · line 1289

Quoted 3 lines · Alexandr Miloslavskiy via GitGitGadget

t/t3903-stash.sh +2 -0

+test_expect_success 'stash handles large files' '
+       x=0123456789abcde\n && # 16

Did you intend for the \n in this assignment to be a literal newline? Every shell with which I tested treats it instead as an escaped 'n'.

t/t3903-stash.sh test_expect_success 'stash handles skip-worktree entries nicely' ' · line 1298

Quoted 9 lines

t/t3903-stash.sh +9 -0

+       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

By the way, are the embedded newlines actually important to the test itself, or are they just for human consumption if the test fails? I ask because I was curious about how other tests create large files, and found that a mechanism similar to your original (but without the pitfalls) has been used. For instance, t1050-large.sh uses:

    printf "%2000000s" X >large1 &&

which is plenty portable and (presumably) doesn't have such demanding memory consumption.

Junio C Hamano

2020-02-17 17:56 UTC #6 lore raw

Quoted 6 lines · Eric Sunshine

... For instance, t1050-large.sh uses:
    printf "%2000000s" X >large1 &&
which is plenty portable and (presumably) doesn't have such demanding memory consumption.

Yes, I had the exact same reaction to echoing large string with literal backslash-en in it ;-) Thanks for reviewing and teaching.

Alexandr Miloslavskiy

2020-02-17 18:02 UTC #7 lore raw

In reply to Eric Sunshine

Quoted 4 lines · Eric Sunshine

Quoted 1 line
+ x=0123456789abcde\n && # 16
Did you intend for the \n in this assignment to be a literal newline? Every shell with which I tested treats it instead as an escaped 'n'.

I'm such a novice shell script writer :(
Yes, I intended a newline.

Quoted 10 lines

By the way, are the embedded newlines actually important to the test itself, or are they just for human consumption if the test fails?I ask because I was curious about how other tests create large files, and found that a mechanism similar to your original (but without the pitfalls) has been used. For instance, t1050-large.sh uses:
     printf "%2000000s" X >large1 &&
which is plenty portable and (presumably) doesn't have such demanding memory consumption.

They are not important to the test; the test only needs to internally have a 8+ mb patch.

This only comes from my feeling that super-large lines could cause other unexpected things, such as hitting various completely reasonable limits and/or causing unwanted slowdowns. Frankly, I didn't test.

Frankly, I already had concerns about adding the test. Now I have re-evaluated things and finally decided to move the test into commit message instead. With it, all compatibility etc questions are resolved.

Alexandr Miloslavskiy via GitGitGadget

author

2020-02-17 18:01 UTC #8 lore raw

PATCH v3 1/1 mingw: workaround for hangs when sending STDIN