threads / patch / 10909

patchfetch-pack: Prepare for a side-band demultiplexer in a thread.

Subject: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

## tl;dr

7 messages between Nov 17, 2007 and Nov 18, 2007. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Johannes Sixt· Nov 17, 2007, 22:09 UTC · lore

get_pack() receives a pair of file descriptors that communicate with upload-pack at the remote end. In order to support the case where the side-band demultiplexer runs in a thread, and, hence, in the same process as the main routine, we must not close the readable file descriptor early.

The handling of the readable fd is changed in the case where upload-pack supports side-band communication: The old code closed the fd after it was inherited to the side-band demultiplexer process. Now we do not close it. The caller (do_fetch_pack) will close it later anyway. The demultiplexer is the only reader, it does not matter that the fd remains open in the main process as well as in unpack-objects/index-pack, which inherits it.

The writable fd is not needed in get_pack(), hence, the old code closed the fd. For symmetry with the readable fd, we now do not close it; the caller (do_fetch_pack) will close it later anyway. Therefore, the new behavior is that the channel now remains open during the entire conversation, but this has no ill effects because upload-pack does not read from it once it has begun to send the pack data. For the same reason it does not matter that the writable fd is now inherited to the demultiplexer and unpack-objects/index-pack processes.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
	This change again originates from the MinGW port. Since we don't
	have fork(2) on Windows, we must run the sideband demultiplexer
	in a thread.
	-- Hannes
 builtin-fetch-pack.c |   42 ++++++++++++++++--------------------------
 1 files changed, 16 insertions(+), 26 deletions(-)
Show changes to builtin-fetch-pack.c +16 −26
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index bb1742f..807fa93 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -462,34 +462,12 @@ static int sideband_demux(int fd, void *data)
 {
 	int *xd = data;
 
-	close(xd[1]);
 	return recv_sideband("fetch-pack", xd[0], fd, 2);
 }
 
-static void setup_sideband(int fd[2], int xd[2], struct async *demux)
-{
-	if (!use_sideband) {
-		fd[0] = xd[0];
-		fd[1] = xd[1];
-		return;
-	}
-	/* xd[] is talking with upload-pack; subprocess reads from
-	 * xd[0], spits out band#2 to stderr, and feeds us band#1
-	 * through demux->out.
-	 */
-	demux->proc = sideband_demux;
-	demux->data = xd;
-	if (start_async(demux))
-		die("fetch-pack: unable to fork off sideband demultiplexer");
-	close(xd[0]);
-	fd[0] = demux->out;
-	fd[1] = xd[1];
-}
-
 static int get_pack(int xd[2], char **pack_lockfile)
 {
 	struct async demux;
-	int fd[2];
 	const char *argv[20];
 	char keep_arg[256];
 	char hdr_arg[256];
@@ -497,7 +475,20 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	int do_keep = args.keep_pack;
 	struct child_process cmd;
 
-	setup_sideband(fd, xd, &demux);
+	memset(&demux, 0, sizeof(demux));
+	if (use_sideband) {
+		/* xd[] is talking with upload-pack; subprocess reads from
+		 * xd[0], spits out band#2 to stderr, and feeds us band#1
+		 * through demux->out.
+		 */
+		demux.proc = sideband_demux;
+		demux.data = xd;
+		if (start_async(&demux))
+			die("fetch-pack: unable to fork off sideband"
+			    " demultiplexer");
+	}
+	else
+		demux.out = xd[0];
 
 	memset(&cmd, 0, sizeof(cmd));
 	cmd.argv = argv;
@@ -506,7 +497,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	if (!args.keep_pack && unpack_limit) {
 		struct pack_header header;
 
-		if (read_pack_header(fd[0], &header))
+		if (read_pack_header(demux.out, &header))
 			die("protocol error: bad pack header");
 		snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
 			 ntohl(header.hdr_version), ntohl(header.hdr_entries));
@@ -542,11 +533,10 @@ static int get_pack(int xd[2], char **pack_lockfile)
 		*av++ = hdr_arg;
 	*av++ = NULL;
 
-	cmd.in = fd[0];
+	cmd.in = demux.out;
 	cmd.git_cmd = 1;
 	if (start_command(&cmd))
 		die("fetch-pack: unable to fork off %s", argv[0]);
-	close(fd[1]);
 	if (do_keep && pack_lockfile)
 		*pack_lockfile = index_pack_lockfile(cmd.out);
 
-- 
1.5.3.5.721.g039b-dirty
Junio C Hamano· Nov 18, 2007, 00:42 UTC · re: Johannes Sixt · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

Johannes Sixt <johannes.sixt@telecom.at> writes:
> 	This change again originates from the MinGW port. Since we don't
> 	have fork(2) on Windows, we must run the sideband demultiplexer
> 	in a thread.

If the rationale was "running in a thread is more natural on the platform", I would understand it.

But "_must_ run because there is no fork(2)" solicits a "Huh? How does Cygwin does it then?" from me.

Alex Riesen· Nov 18, 2007, 01:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

Junio C Hamano, Sun, Nov 18, 2007 01:42:11 +0100:
Show 12 quoted lines
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
> > 	This change again originates from the MinGW port. Since we don't
> > 	have fork(2) on Windows, we must run the sideband demultiplexer
> > 	in a thread.
> 
> If the rationale was "running in a thread is more natural on the
> platform", I would understand it.
> 
> But "_must_ run because there is no fork(2)" solicits a "Huh?
> How does Cygwin does it then?" from me.
> 

You wont believe it: they start the currently running program again and copy parents memory over into the child. Sometimes it fails. If you want something scary: http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/fork.cc?rev=1.193&content-type=text/x-cvsweb-markup&cvsroot=src

Johannes Sixt· Nov 18, 2007, 09:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

On Sunday 18 November 2007 01:42, Junio C Hamano wrote:
Show 7 quoted lines
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > 	This change again originates from the MinGW port. Since we don't
> > 	have fork(2) on Windows, we must run the sideband demultiplexer
> > 	in a thread.
>
> If the rationale was "running in a thread is more natural on the
> platform", I would understand it.
Please take it as such.
> But "_must_ run because there is no fork(2)" solicits a "Huh?
> How does Cygwin does it then?" from me.
Alex has answered this. We are not going to copy Cygwin's fork() into git.
-- Hannes
Junio C Hamano· Nov 18, 2007, 10:16 UTC · re: Johannes Sixt · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

Johannes Sixt <johannes.sixt@telecom.at> writes:
Show 9 quoted lines
>> If the rationale was "running in a thread is more natural on the
>> platform", I would understand it.
>
> Please take it as such.
>
>> But "_must_ run because there is no fork(2)" solicits a "Huh?
>> How does Cygwin does it then?" from me.
>
> Alex has answered this. We are not going to copy Cygwin's fork() into git.

Wholeheartedly agree, and I feel dirty after looking at that thing. Nothing against Cygwin folks, of course.

The reason I wanted to close pipes as early as possible was because I did not want to get bitten by "read() waiting forever for EOF due to an extra unclosed fd on the writer end of the pipe() without the writer not writing anything to it" problem, which is an often-seen mistake.

As long as you did not introduce that problem the change is fine by me.

Johannes Sixt· Nov 18, 2007, 12:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

On Sunday 18 November 2007 11:16, Junio C Hamano wrote:
Show 20 quoted lines
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> >> If the rationale was "running in a thread is more natural on the
> >> platform", I would understand it.
> >
> > Please take it as such.
> >
> >> But "_must_ run because there is no fork(2)" solicits a "Huh?
> >> How does Cygwin does it then?" from me.
> >
> > Alex has answered this. We are not going to copy Cygwin's fork() into
> > git.
>
> Wholeheartedly agree, and I feel dirty after looking at that
> thing.  Nothing against Cygwin folks, of course.
>
> The reason I wanted to close pipes as early as possible was
> because I did not want to get bitten by "read() waiting forever
> for EOF due to an extra unclosed fd on the writer end of the
> pipe() without the writer not writing anything to it" problem,
> which is an often-seen mistake.

That was my concern, too, and I spent quite a few brain cycles to verify that this is not the case here.

-- Hannes
Robin Rosenberg· Nov 18, 2007, 11:03 UTC · re: Johannes Sixt · lore

Re: [PATCH] fetch-pack: Prepare for a side-band demultiplexer in a thread.

lördag 17 november 2007 skrev Johannes Sixt:
Show 5 quoted lines
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
> 	This change again originates from the MinGW port. Since we don't
> 	have fork(2) on Windows, we must run the sideband demultiplexer
> 	in a thread.
I think this explanation deserves a place in the commit too.
-- robin

← back to recent threads