[PATCH] CYGWIN: Use a TCP socket for pipe()

[PATCH] CYGWIN: Use a TCP socket for pipe()

[Torsten Bögershausen]

Work around issues that git hangs when doing fetch or pull under
various protocols under CYGWIN.

Replace pipe() with a socket connection using a TCP/IP.
Introduce a new function socket_pipe() in compat/socket_pipe.c

Re-define the pipe() function into socket_pipe() for CYGWIN.

(This redefinition does not work for MinGW, because here a socket can
not be mixed with a file handle)

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/socket_pipe.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 config.mak.uname     |  2 ++
 git-compat-util.h    |  4 ++++
 3 files changed, 74 insertions(+)
 create mode 100644 compat/socket_pipe.c

diff --git a/compat/socket_pipe.c b/compat/socket_pipe.c
new file mode 100644
index 0000000..eee43b5
--- /dev/null
+++ b/compat/socket_pipe.c
@@ -0,0 +1,68 @@
+#include "git-compat-util.h"
+
+int socket_pipe(int filedes[2])
+{
+	int fd_listen, fd_rd;
+	int fd_wr = -1;
+	struct sockaddr_in sin;
+	socklen_t length;
+	struct linger linger;
+	int reuse_on = 1;
+
+	memset(&sin, 0, sizeof(sin));
+	sin.sin_family = AF_INET;
+	sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+
+	fd_listen = socket(AF_INET, SOCK_STREAM, 0);
+	if (fd_listen == -1)
+		die_errno("pipe fd_listen socket");
+	(void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,
+									 (char*)&reuse_on, sizeof(reuse_on));
+	if (bind(fd_listen, (struct sockaddr *)&sin, sizeof(sin)))
+		die_errno("pipe bind socket");
+
+	length = sizeof(sin);
+	if(getsockname(fd_listen, (struct sockaddr *)&sin, &length))
+		die_errno("pipe getsockname");
+	if (listen(fd_listen, SOMAXCONN))
+		die_errno("pipe listen");
+	fd_rd = socket(AF_INET, SOCK_STREAM, 0);
+	if (fd_rd == -1)
+		die_errno("pipe fd_rd socket");
+	if (connect(fd_rd, (struct sockaddr *)&sin, sizeof(sin)))
+		die_errno("pipe connect");
+
+	length = sizeof(struct sockaddr);
+	if(getsockname(fd_rd, (struct sockaddr *)&sin, &length))
+		die_errno("pipe getsockname");
+
+	while (fd_wr == -1) {
+		struct sockaddr_in sacc;
+		length = sizeof(sacc);
+		memset(&sacc, 0, sizeof(sacc));
+		fd_wr = accept(fd_listen, (struct sockaddr *)&sacc, &length);
+		if(fd_wr == -1)
+			die_errno("pipe accept");
+		if (sacc.sin_port != sin.sin_port) {
+			// Wrong connecting socket
+			close(fd_wr);
+			fd_wr = -1;
+		}
+	}
+
+	close(fd_listen);
+
+	linger.l_onoff = 1;
+	linger.l_linger = 5;
+	if (setsockopt(fd_wr, SOL_SOCKET, SO_LINGER,
+								 (char*)&linger, sizeof(linger)))
+		die_errno("pipe socket linger");
+	if (shutdown(fd_rd, SHUT_WR))
+		die_errno("pipe socket shutdown fd_rd");
+	if (shutdown(fd_wr, SHUT_RD))
+		die_errno("pipe socket shutdown fd_wr");
+
+	filedes[0] = fd_rd;
+	filedes[1] = fd_wr;
+	return 0;
+}
diff --git a/config.mak.uname b/config.mak.uname
index d78fd3d..66bf446 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -170,7 +170,9 @@ ifeq ($(uname_O),Cygwin)
 	# Try commenting this out if you suspect MMAP is more efficient
 	NO_MMAP = YesPlease
 	X = .exe
+	BASIC_CFLAGS += -DGIT_USE_SOCKET_PIPE
 	COMPAT_OBJS += compat/cygwin.o
+	COMPAT_OBJS += compat/socket_pipe.o
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 endif
diff --git a/git-compat-util.h b/git-compat-util.h
index c1f8a47..88632ab 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -176,6 +176,10 @@ int get_st_mode_bits(const char *path, int *mode);
 #endif
 #endif
 
+int socket_pipe(int filedes[2]);
+#ifdef GIT_USE_SOCKET_PIPE
+#define pipe(a) socket_pipe(a)
+#endif
 /* used on Mac OS X */
 #ifdef PRECOMPOSE_UNICODE
 #include "compat/precompose_utf8.h"
-- 
1.8.3

Re: [PATCH] CYGWIN: Use a TCP socket for pipe()

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Work around issues that git hangs when doing fetch or pull under
> various protocols under CYGWIN.
>
> Replace pipe() with a socket connection using a TCP/IP.
> Introduce a new function socket_pipe() in compat/socket_pipe.c

Sounds like sweeping the real problem, whatever it is, under rug.
Is it that we are assuming a pipe buffer that is sufficiently large
and expecting a write that we deem to be small enough not to block,
causing a deadlock on a platform with very small pipe buffer, or
something?
> +	(void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,
> +									 (char*)&reuse_on, sizeof(reuse_on));

Broken indentation.

Either align (note: these are only to illustrate the column
alignment; I am not encouraging to indent with leading SPs) the
opening "(" of the "(char *)" cast with "f" in "fd_listen", like
this,

        (void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,
                         (char *)&reuse_on, sizeof(reuse_on));

by a run of HT with minimum number of SP, or push the beginning of
the second line past opening "(" after "setsockopt", like this,

        (void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,
                                (char *)&reuse_on, sizeof(reuse_on));

with a run of minimum number of HT without any SP.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c1f8a47..88632ab 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -176,6 +176,10 @@ int get_st_mode_bits(const char *path, int *mode);
>  #endif
>  #endif
>  
> +int socket_pipe(int filedes[2]);
> +#ifdef GIT_USE_SOCKET_PIPE
> +#define pipe(a) socket_pipe(a)
> +#endif

Shouldn't the function declaration be inside this #ifdef?

>  /* used on Mac OS X */
>  #ifdef PRECOMPOSE_UNICODE
>  #include "compat/precompose_utf8.h"

Re: [PATCH] CYGWIN: Use a TCP socket for pipe()

[Mark Levedahl]

On 06/27/2013 01:38 PM, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Work around issues that git hangs when doing fetch or pull under
>> various protocols under CYGWIN.
>>
>> Replace pipe() with a socket connection using a TCP/IP.
>> Introduce a new function socket_pipe() in compat/socket_pipe.c
> Sounds like sweeping the real problem, whatever it is, under rug.
> Is it that we are assuming a pipe buffer that is sufficiently large
> and expecting a write that we deem to be small enough not to block,
> causing a deadlock on a platform with very small pipe buffer, or
> something?
>

There were issues in early v1.7 Cygwin release for overlapping I/O such 
that the pipe was sometimes terminated early resulting in data loss. If 
the pipe implementation in Cygwin is still a problem a good test case 
sent to the Cygwin developers would be the right approach rather than a 
one-off patch in git to try to work around a current platform bug.

Note - I do not see random hangs with the stat/lstat hack removed, 
rather the sole test suite hang I see is repeatable in t0008.sh. So, if 
the patch remains, we may be able to run this remaining hang to ground.

Mark

Re: [PATCH] CYGWIN: Use a TCP socket for pipe()

[Torsten Bögershausen]

On 2013-06-28 04.46, Mark Levedahl wrote:
> On 06/27/2013 01:38 PM, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> Work around issues that git hangs when doing fetch or pull under
>>> various protocols under CYGWIN.
>>>
>>> Replace pipe() with a socket connection using a TCP/IP.
>>> Introduce a new function socket_pipe() in compat/socket_pipe.c
>> Sounds like sweeping the real problem, whatever it is, under rug.
>> Is it that we are assuming a pipe buffer that is sufficiently large
>> and expecting a write that we deem to be small enough not to block,
>> causing a deadlock on a platform with very small pipe buffer, or
>> something?
>>
>
> There were issues in early v1.7 Cygwin release for overlapping I/O such that the pipe was sometimes terminated early resulting in data loss. If the pipe implementation in Cygwin is still a problem a good test case sent to the Cygwin developers would be the right approach rather than a one-off patch in git to try to work around a current platform bug.
>
> Note - I do not see random hangs with the stat/lstat hack removed, rather the sole test suite hang I see is repeatable in t0008.sh. So, if the patch remains, we may be able to run this remaining hang to ground.
>
> Mark
Thanks,
I testet "rj/cygwin-remove-cheating-lstat" with the "socket pipe" on top:
no hanging.

Then I run "rj/cygwin-remove-cheating-lstat" without "socket pipe",
(or in other words git.git/pu):
No hanging.

Then I run a "stress test" with many (but not all) "git fetch" tests:
 t1507, t1514, t2015, t2024, t3200, t3409, t3600, t4041, t6050, t6200
repeat those tests in a forever loop.

All these test run 24 hours in a row on a single core machine, no hanging.
(I need to re-do the test on a dual-core machine)

So at the moment I don't have any problems to report for cygwin, which is good.

And it looks as if "rj/cygwin-remove-cheating-lstat" prevents the "hanging",
so there is another +1 to keep it and move it into next.
/Torsten

Re: [PATCH] CYGWIN: Use a TCP socket for pipe()

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> I testet "rj/cygwin-remove-cheating-lstat" with the "socket pipe" on top:
> no hanging.
>
> Then I run "rj/cygwin-remove-cheating-lstat" without "socket pipe",
> (or in other words git.git/pu):
> No hanging.

So an immediate conclusion is that we can forget about this patch?

> So at the moment I don't have any problems to report for cygwin, which is good.
>
> And it looks as if "rj/cygwin-remove-cheating-lstat" prevents the "hanging",
> so there is another +1 to keep it and move it into next.

Ramsay started a "mark places we call lstat() when we do not really
need fully correct lstat" topic, and I think it may be a sane
direction to go, as long as the helper function's semantic is
clearly defined.

It would be worth seeing where it leads us, before ripping that
"cheating and incomplete lstat out, I think.

Re: [PATCH] CYGWIN: Use a TCP socket for pipe()

[Torsten Bögershausen]

On 2013-07-01 00.56, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> I testet "rj/cygwin-remove-cheating-lstat" with the "socket pipe" on top:
>> no hanging.
>>
>> Then I run "rj/cygwin-remove-cheating-lstat" without "socket pipe",
>> (or in other words git.git/pu):
>> No hanging.
> 
> So an immediate conclusion is that we can forget about this patch?
Yes
> 
>> So at the moment I don't have any problems to report for cygwin, which is good.
>>
>> And it looks as if "rj/cygwin-remove-cheating-lstat" prevents the "hanging",
>> so there is another +1 to keep it and move it into next.
> 
> Ramsay started a "mark places we call lstat() when we do not really
> need fully correct lstat" topic, and I think it may be a sane
> direction to go, as long as the helper function's semantic is
> clearly defined.
> 
> It would be worth seeing where it leads us, before ripping that
> "cheating and incomplete lstat out, I think.
I currently run the test suite on it, based on next.
Got one hanging, but of a different kind:
git was running fetch, but the corresponding git-upload-pack.exe was not in the
task list of the windows explorer, but it was in ps under cygwin.


diff --git a/run-command.c b/run-command.c
index aece872..ee588eb 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,6 +226,9 @@ static inline void set_cloexec(int fd)
                fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
+#if defined(__CYGWIN__)
+#define wait_or_whine(p, a) (p)
+#else
 static int wait_or_whine(pid_t pid, const char *argv0)
 {
        int status, code = -1;
@@ -268,6 +271,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
        errno = failed_errno;
        return code;
 }
+#endif

(And I needed to remove the credential helper tests)

More info later
/Torsten