Skip to contentSkip to conversation

simple-ipc: work around issues with Cygwin's Unix socket emulation

PATCH In review Johannes Schindelin via GitGitGadget 3 participants 3 messages Nov 10, 2021 – Nov 12, 2021

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 simple-ipc: work around issues with Cygwin's Unix socket emulation

From: Johannes Schindelin <[email protected]>

Cygwin emulates Unix sockets by writing files with custom contents and then marking them as system files.

The tricky problem is that while the file is written and its system bit is set, it is still identified as a file. This caused test failures when Git is too fast looking for the Unix sockets and then complains that there is a plain file in the way.

Let's work around this by adding a delayed retry loop, specifically for Cygwin.

---
simple-ipc: work around issues with Cygwin's Unix socket emulation

Adam Dinwoodie reported problems running the simple-ipc tests on Cygwin
[https://lore.kernel.org/git/[email protected]]. This patch works around the underlying problem, which is rooted in Cygwin's implementation details.

With this patch, I could not reproduce the problem, even with sh t0052-simple-ipc.sh --stress-limit=30.

As per Junio's encouragement
[https://lore.kernel.org/git/[email protected]], I am submitting this still in the -rc phase, hoping that it will make it into v2.34.0 final.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1074%2Fdscho%2Fcygwin-vs-simple-ipc-workaround-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1074/dscho/cygwin-vs-simple-ipc-workaround-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1074

 compat/simple-ipc/ipc-unix-socket.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
compat/simple-ipc/ipc-unix-socket.c +22 -0
index 4e28857a0a1..28a79289d4f 100644
@@ -35,6 +35,28 @@ enum ipc_active_state ipc_get_active_state(const char *path)
 		}
 	}
 
+#ifdef __CYGWIN__
+	/*
+	 * Cygwin emulates Unix sockets by writing special-crafted files whose
+	 * `system` bit is set.
+	 *
+	 * If we are too fast, Cygwin might still be in the process of marking
+	 * the underlying file as a system file. Until then, we will not see a
+	 * Unix socket here, but a plain file instead. Just in case that this
+	 * is happening, wait a little and try again.
+	 */
+	{
+		static const int delay[] = { 1, 10, 20, 40, -1 };
+		int i;
+
+		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
+			sleep_millisec(delay[i]);
+			if (lstat(path, &st) == -1)
+				return IPC_STATE__INVALID_PATH;
+		}
+	}
+#endif
+
 	/* also complain if a plain file is in the way */
 	if ((st.st_mode & S_IFMT) != S_IFSOCK)
 		return IPC_STATE__INVALID_PATH;

base-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b

Signature · 1 line
gitgitgadget
#1 lore raw

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 10 lines · writes:

From: Johannes Schindelin <[email protected]>

Cygwin emulates Unix sockets by writing files with custom contents and then marking them as system files.

The tricky problem is that while the file is written and its system bit is set, it is still identified as a file. This caused test failures when Git is too fast looking for the Unix sockets and then complains that there is a plain file in the way.

Quoted 5 lines

Let's work around this by adding a delayed retry loop, specifically for Cygwin.

---

OK, I was about to ask for an Ack from Cygwin folks but I see the original that got Ack is more-or-less the same except for placement of the comments (and this version of course is more polished than the "this should work---please try it" version), so let me pretend that this got tested-by from those who were happy with the original in the old thread.

Thanks.

Quoted 57 lines

simple-ipc: work around issues with Cygwin's Unix socket emulation

Adam Dinwoodie reported problems running the simple-ipc tests on Cygwin
[https://lore.kernel.org/git/[email protected]]. This patch works around the underlying problem, which is rooted in Cygwin's implementation details.

With this patch, I could not reproduce the problem, even with sh t0052-simple-ipc.sh --stress-limit=30.

As per Junio's encouragement
[https://lore.kernel.org/git/[email protected]], I am submitting this still in the -rc phase, hoping that it will make it into v2.34.0 final.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1074%2Fdscho%2Fcygwin-vs-simple-ipc-workaround-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1074/dscho/cygwin-vs-simple-ipc-workaround-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1074

 compat/simple-ipc/ipc-unix-socket.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
compat/simple-ipc/ipc-unix-socket.c +22 -0
index 4e28857a0a1..28a79289d4f 100644
@@ -35,6 +35,28 @@ enum ipc_active_state ipc_get_active_state(const char *path)
 		}
 	}
 
+#ifdef __CYGWIN__
+	/*
+	 * Cygwin emulates Unix sockets by writing special-crafted files whose
+	 * `system` bit is set.
+	 *
+	 * If we are too fast, Cygwin might still be in the process of marking
+	 * the underlying file as a system file. Until then, we will not see a
+	 * Unix socket here, but a plain file instead. Just in case that this
+	 * is happening, wait a little and try again.
+	 */
+	{
+		static const int delay[] = { 1, 10, 20, 40, -1 };
+		int i;
+
+		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
+			sleep_millisec(delay[i]);
+			if (lstat(path, &st) == -1)
+				return IPC_STATE__INVALID_PATH;
+		}
+	}
+#endif
+
 	/* also complain if a plain file is in the way */
 	if ((st.st_mode & S_IFMT) != S_IFSOCK)
 		return IPC_STATE__INVALID_PATH;

base-commit: 6c220937e2b26d85920bf2d38ff2464a0d57fd6b

#2 lore raw
Quoted 20 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 10 lines · writes:

From: Johannes Schindelin <[email protected]>

Cygwin emulates Unix sockets by writing files with custom contents and then marking them as system files.

The tricky problem is that while the file is written and its system bit is set, it is still identified as a file. This caused test failures when Git is too fast looking for the Unix sockets and then complains that there is a plain file in the way.

Quoted 5 lines

Let's work around this by adding a delayed retry loop, specifically for Cygwin.

---

Quoted 6 lines

OK, I was about to ask for an Ack from Cygwin folks but I see the original that got Ack is more-or-less the same except for placement of the comments (and this version of course is more polished than the "this should work---please try it" version), so let me pretend that this got tested-by from those who were happy with the original in the old thread.

Late to the party – other bits of life got in the way – but I've just confirmed this appears to resolve the issue for me too.

Adam