{"thread":{"id":"34289","subject":"[PATCH] CYGWIN: Use a TCP socket for pipe()","startedAt":"2013-06-27T16:31:13Z","lastAt":"2013-07-02T14:57:21Z","messageCount":6,"participants":["Torsten Bögershausen","Junio C Hamano","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"222094","messageId":"201306271831.13865.tboegi@web.de","threadId":"34289","inReplyTo":null,"subject":"[PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-27T16:31:13Z","receivedAt":"2013-06-27T16:31:13Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Work around issues that git hangs when doing fetch or pull under\nvarious protocols under CYGWIN.\n\nReplace pipe() with a socket connection using a TCP/IP.\nIntroduce a new function socket_pipe() in compat/socket_pipe.c\n\nRe-define the pipe() function into socket_pipe() for CYGWIN.\n\n(This redefinition does not work for MinGW, because here a socket can\nnot be mixed with a file handle)\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n compat/socket_pipe.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n config.mak.uname     |  2 ++\n git-compat-util.h    |  4 ++++\n 3 files changed, 74 insertions(+)\n create mode 100644 compat/socket_pipe.c\n\ndiff --git a/compat/socket_pipe.c b/compat/socket_pipe.c\nnew file mode 100644\nindex 0000000..eee43b5\n--- /dev/null\n+++ b/compat/socket_pipe.c\n@@ -0,0 +1,68 @@\n+#include \"git-compat-util.h\"\n+\n+int socket_pipe(int filedes[2])\n+{\n+\tint fd_listen, fd_rd;\n+\tint fd_wr = -1;\n+\tstruct sockaddr_in sin;\n+\tsocklen_t length;\n+\tstruct linger linger;\n+\tint reuse_on = 1;\n+\n+\tmemset(&sin, 0, sizeof(sin));\n+\tsin.sin_family = AF_INET;\n+\tsin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);\n+\n+\tfd_listen = socket(AF_INET, SOCK_STREAM, 0);\n+\tif (fd_listen == -1)\n+\t\tdie_errno(\"pipe fd_listen socket\");\n+\t(void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,\n+\t\t\t\t\t\t\t\t\t (char*)&reuse_on, sizeof(reuse_on));\n+\tif (bind(fd_listen, (struct sockaddr *)&sin, sizeof(sin)))\n+\t\tdie_errno(\"pipe bind socket\");\n+\n+\tlength = sizeof(sin);\n+\tif(getsockname(fd_listen, (struct sockaddr *)&sin, &length))\n+\t\tdie_errno(\"pipe getsockname\");\n+\tif (listen(fd_listen, SOMAXCONN))\n+\t\tdie_errno(\"pipe listen\");\n+\tfd_rd = socket(AF_INET, SOCK_STREAM, 0);\n+\tif (fd_rd == -1)\n+\t\tdie_errno(\"pipe fd_rd socket\");\n+\tif (connect(fd_rd, (struct sockaddr *)&sin, sizeof(sin)))\n+\t\tdie_errno(\"pipe connect\");\n+\n+\tlength = sizeof(struct sockaddr);\n+\tif(getsockname(fd_rd, (struct sockaddr *)&sin, &length))\n+\t\tdie_errno(\"pipe getsockname\");\n+\n+\twhile (fd_wr == -1) {\n+\t\tstruct sockaddr_in sacc;\n+\t\tlength = sizeof(sacc);\n+\t\tmemset(&sacc, 0, sizeof(sacc));\n+\t\tfd_wr = accept(fd_listen, (struct sockaddr *)&sacc, &length);\n+\t\tif(fd_wr == -1)\n+\t\t\tdie_errno(\"pipe accept\");\n+\t\tif (sacc.sin_port != sin.sin_port) {\n+\t\t\t// Wrong connecting socket\n+\t\t\tclose(fd_wr);\n+\t\t\tfd_wr = -1;\n+\t\t}\n+\t}\n+\n+\tclose(fd_listen);\n+\n+\tlinger.l_onoff = 1;\n+\tlinger.l_linger = 5;\n+\tif (setsockopt(fd_wr, SOL_SOCKET, SO_LINGER,\n+\t\t\t\t\t\t\t\t (char*)&linger, sizeof(linger)))\n+\t\tdie_errno(\"pipe socket linger\");\n+\tif (shutdown(fd_rd, SHUT_WR))\n+\t\tdie_errno(\"pipe socket shutdown fd_rd\");\n+\tif (shutdown(fd_wr, SHUT_RD))\n+\t\tdie_errno(\"pipe socket shutdown fd_wr\");\n+\n+\tfiledes[0] = fd_rd;\n+\tfiledes[1] = fd_wr;\n+\treturn 0;\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex d78fd3d..66bf446 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -170,7 +170,9 @@ ifeq ($(uname_O),Cygwin)\n \t# Try commenting this out if you suspect MMAP is more efficient\n \tNO_MMAP = YesPlease\n \tX = .exe\n+\tBASIC_CFLAGS += -DGIT_USE_SOCKET_PIPE\n \tCOMPAT_OBJS += compat/cygwin.o\n+\tCOMPAT_OBJS += compat/socket_pipe.o\n \tUNRELIABLE_FSTAT = UnfortunatelyYes\n \tSPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield\n endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c1f8a47..88632ab 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -176,6 +176,10 @@ int get_st_mode_bits(const char *path, int *mode);\n #endif\n #endif\n \n+int socket_pipe(int filedes[2]);\n+#ifdef GIT_USE_SOCKET_PIPE\n+#define pipe(a) socket_pipe(a)\n+#endif\n /* used on Mac OS X */\n #ifdef PRECOMPOSE_UNICODE\n #include \"compat/precompose_utf8.h\"\n-- \n1.8.3\n"},{"id":"222111","messageId":"7vvc4zsbbs.fsf@alter.siamese.dyndns.org","threadId":"34289","inReplyTo":"201306271831.13865.tboegi@web.de","subject":"Re: [PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-27T17:38:47Z","receivedAt":"2013-06-27T17:38:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Work around issues that git hangs when doing fetch or pull under\n> various protocols under CYGWIN.\n>\n> Replace pipe() with a socket connection using a TCP/IP.\n> Introduce a new function socket_pipe() in compat/socket_pipe.c\n\nSounds like sweeping the real problem, whatever it is, under rug.\nIs it that we are assuming a pipe buffer that is sufficiently large\nand expecting a write that we deem to be small enough not to block,\ncausing a deadlock on a platform with very small pipe buffer, or\nsomething?\n> +\t(void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,\n> +\t\t\t\t\t\t\t\t\t (char*)&reuse_on, sizeof(reuse_on));\n\nBroken indentation.\n\nEither align (note: these are only to illustrate the column\nalignment; I am not encouraging to indent with leading SPs) the\nopening \"(\" of the \"(char *)\" cast with \"f\" in \"fd_listen\", like\nthis,\n\n        (void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,\n                         (char *)&reuse_on, sizeof(reuse_on));\n\nby a run of HT with minimum number of SP, or push the beginning of\nthe second line past opening \"(\" after \"setsockopt\", like this,\n\n        (void)setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,\n                                (char *)&reuse_on, sizeof(reuse_on));\n\nwith a run of minimum number of HT without any SP.\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index c1f8a47..88632ab 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -176,6 +176,10 @@ int get_st_mode_bits(const char *path, int *mode);\n>  #endif\n>  #endif\n>  \n> +int socket_pipe(int filedes[2]);\n> +#ifdef GIT_USE_SOCKET_PIPE\n> +#define pipe(a) socket_pipe(a)\n> +#endif\n\nShouldn't the function declaration be inside this #ifdef?\n\n>  /* used on Mac OS X */\n>  #ifdef PRECOMPOSE_UNICODE\n>  #include \"compat/precompose_utf8.h\"\n"},{"id":"222150","messageId":"51CCF91F.7080501@gmail.com","threadId":"34289","inReplyTo":"7vvc4zsbbs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2013-06-28T02:46:55Z","receivedAt":"2013-06-28T02:46:55Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"On 06/27/2013 01:38 PM, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n>\n>> Work around issues that git hangs when doing fetch or pull under\n>> various protocols under CYGWIN.\n>>\n>> Replace pipe() with a socket connection using a TCP/IP.\n>> Introduce a new function socket_pipe() in compat/socket_pipe.c\n> Sounds like sweeping the real problem, whatever it is, under rug.\n> Is it that we are assuming a pipe buffer that is sufficiently large\n> and expecting a write that we deem to be small enough not to block,\n> causing a deadlock on a platform with very small pipe buffer, or\n> something?\n>\n\nThere were issues in early v1.7 Cygwin release for overlapping I/O such \nthat the pipe was sometimes terminated early resulting in data loss. If \nthe pipe implementation in Cygwin is still a problem a good test case \nsent to the Cygwin developers would be the right approach rather than a \none-off patch in git to try to work around a current platform bug.\n\nNote - I do not see random hangs with the stat/lstat hack removed, \nrather the sole test suite hang I see is repeatable in t0008.sh. So, if \nthe patch remains, we may be able to run this remaining hang to ground.\n\nMark\n"},{"id":"222262","messageId":"51D08B58.8000103@web.de","threadId":"34289","inReplyTo":"51CCF91F.7080501@gmail.com","subject":"Re: [PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-06-30T19:47:36Z","receivedAt":"2013-06-30T19:47:36Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-06-28 04.46, Mark Levedahl wrote:\n> On 06/27/2013 01:38 PM, Junio C Hamano wrote:\n>> Torsten Bögershausen <tboegi@web.de> writes:\n>>\n>>> Work around issues that git hangs when doing fetch or pull under\n>>> various protocols under CYGWIN.\n>>>\n>>> Replace pipe() with a socket connection using a TCP/IP.\n>>> Introduce a new function socket_pipe() in compat/socket_pipe.c\n>> Sounds like sweeping the real problem, whatever it is, under rug.\n>> Is it that we are assuming a pipe buffer that is sufficiently large\n>> and expecting a write that we deem to be small enough not to block,\n>> causing a deadlock on a platform with very small pipe buffer, or\n>> something?\n>>\n>\n> 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.\n>\n> 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.\n>\n> Mark\nThanks,\nI testet \"rj/cygwin-remove-cheating-lstat\" with the \"socket pipe\" on top:\nno hanging.\n\nThen I run \"rj/cygwin-remove-cheating-lstat\" without \"socket pipe\",\n(or in other words git.git/pu):\nNo hanging.\n\nThen I run a \"stress test\" with many (but not all) \"git fetch\" tests:\n t1507, t1514, t2015, t2024, t3200, t3409, t3600, t4041, t6050, t6200\nrepeat those tests in a forever loop.\n\nAll these test run 24 hours in a row on a single core machine, no hanging.\n(I need to re-do the test on a dual-core machine)\n\nSo at the moment I don't have any problems to report for cygwin, which is good.\n\nAnd it looks as if \"rj/cygwin-remove-cheating-lstat\" prevents the \"hanging\",\nso there is another +1 to keep it and move it into next.\n/Torsten\n"},{"id":"222264","messageId":"7vwqpbi4ws.fsf@alter.siamese.dyndns.org","threadId":"34289","inReplyTo":"51D08B58.8000103@web.de","subject":"Re: [PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-30T22:56:35Z","receivedAt":"2013-06-30T22:56:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> I testet \"rj/cygwin-remove-cheating-lstat\" with the \"socket pipe\" on top:\n> no hanging.\n>\n> Then I run \"rj/cygwin-remove-cheating-lstat\" without \"socket pipe\",\n> (or in other words git.git/pu):\n> No hanging.\n\nSo an immediate conclusion is that we can forget about this patch?\n\n> So at the moment I don't have any problems to report for cygwin, which is good.\n>\n> And it looks as if \"rj/cygwin-remove-cheating-lstat\" prevents the \"hanging\",\n> so there is another +1 to keep it and move it into next.\n\nRamsay started a \"mark places we call lstat() when we do not really\nneed fully correct lstat\" topic, and I think it may be a sane\ndirection to go, as long as the helper function's semantic is\nclearly defined.\n\nIt would be worth seeing where it leads us, before ripping that\n\"cheating and incomplete lstat out, I think.\n"},{"id":"222371","messageId":"51D2EA51.5000807@web.de","threadId":"34289","inReplyTo":"7vwqpbi4ws.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] CYGWIN: Use a TCP socket for pipe()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-07-02T14:57:21Z","receivedAt":"2013-07-02T14:57:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-07-01 00.56, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n>> I testet \"rj/cygwin-remove-cheating-lstat\" with the \"socket pipe\" on top:\n>> no hanging.\n>>\n>> Then I run \"rj/cygwin-remove-cheating-lstat\" without \"socket pipe\",\n>> (or in other words git.git/pu):\n>> No hanging.\n> \n> So an immediate conclusion is that we can forget about this patch?\nYes\n> \n>> So at the moment I don't have any problems to report for cygwin, which is good.\n>>\n>> And it looks as if \"rj/cygwin-remove-cheating-lstat\" prevents the \"hanging\",\n>> so there is another +1 to keep it and move it into next.\n> \n> Ramsay started a \"mark places we call lstat() when we do not really\n> need fully correct lstat\" topic, and I think it may be a sane\n> direction to go, as long as the helper function's semantic is\n> clearly defined.\n> \n> It would be worth seeing where it leads us, before ripping that\n> \"cheating and incomplete lstat out, I think.\nI currently run the test suite on it, based on next.\nGot one hanging, but of a different kind:\ngit was running fetch, but the corresponding git-upload-pack.exe was not in the\ntask list of the windows explorer, but it was in ps under cygwin.\n\n\ndiff --git a/run-command.c b/run-command.c\nindex aece872..ee588eb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,6 +226,9 @@ static inline void set_cloexec(int fd)\n                fcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n+#if defined(__CYGWIN__)\n+#define wait_or_whine(p, a) (p)\n+#else\n static int wait_or_whine(pid_t pid, const char *argv0)\n {\n        int status, code = -1;\n@@ -268,6 +271,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n        errno = failed_errno;\n        return code;\n }\n+#endif\n\n(And I needed to remove the credential helper tests)\n\nMore info later\n/Torsten\n"}]}