git-upload-pack: the timeout gets corrupted?!

git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

git-1.5.0.3-1.i386 rpm from Junio's repository on kernel.org:

Since we got the new git server on kernel.org, we are having a problem 
with git-upload-pack processes getting reparented to init, and then 
sitting there forever.  Going in with gdb, it appears the "timeout" 
variable gets overwritten:

(gdb) p timeout
$1 = 608471321

... which should have been 600.

The process spends effectively forever waiting in on the fflush() in 
show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write 
to a pipe, but I'm not sure what is at the other end of that same pipe.

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> ...  Going in with gdb, it appears the "timeout"
> variable gets overwritten:
>
> (gdb) p timeout
> $1 = 608471321
>
> ... which should have been 600.

I do not see offhand what can cause this.  The only new code
since 1.4.4 series is "shallow clone" stuff,...

> The process spends effectively forever waiting in on the fflush() in
> show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to
> write to a pipe, but I'm not sure what is at the other end of that
> same pipe.

The process forks and the one that runs show_commit() is running
rev-list internally while the other end is a pack-objects that
reads from it and sends its output back to the client.

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

Junio C Hamano wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> ...  Going in with gdb, it appears the "timeout"
>> variable gets overwritten:
>>
>> (gdb) p timeout
>> $1 = 608471321
>>
>> ... which should have been 600.
> 
> I do not see offhand what can cause this.  The only new code
> since 1.4.4 series is "shallow clone" stuff,...
> 
>> The process spends effectively forever waiting in on the fflush() in
>> show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to
>> write to a pipe, but I'm not sure what is at the other end of that
>> same pipe.
> 
> The process forks and the one that runs show_commit() is running
> rev-list internally while the other end is a pack-objects that
> reads from it and sends its output back to the client.
> 

Yup; the pack-objects process has apparently died.

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

Junio C Hamano wrote:
> 
>> The process spends effectively forever waiting in on the fflush() in
>> show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to
>> write to a pipe, but I'm not sure what is at the other end of that
>> same pipe.
> 
> The process forks and the one that runs show_commit() is running
> rev-list internally while the other end is a pack-objects that
> reads from it and sends its output back to the client.
> 

Now, given that the fact that the git-pack-object process has already 
died, normally one would expect the write() to get SIGPIPE which would 
kill the process.  Does git-upload-pack not close the read end of the 
pipe in the writer?  From the looks of the fd directory, I would say it 
does not.

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[Johannes Schindelin]

Hi,

On Sun, 11 Mar 2007, H. Peter Anvin wrote:

> Junio C Hamano wrote:
> > 
> > > The process spends effectively forever waiting in on the fflush() in
> > > show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to
> > > write to a pipe, but I'm not sure what is at the other end of that
> > > same pipe.
> > 
> > The process forks and the one that runs show_commit() is running
> > rev-list internally while the other end is a pack-objects that
> > reads from it and sends its output back to the client.
> > 
> 
> Now, given that the fact that the git-pack-object process has already died,
> normally one would expect the write() to get SIGPIPE which would kill the
> process.  Does git-upload-pack not close the read end of the pipe in the
> writer?  From the looks of the fd directory, I would say it does not.

Something like this (totally untested):

 upload-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 498bf50..bafd90f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -119,6 +119,8 @@ static void create_pack_file(void)
 		int i;
 		struct rev_info revs;
 
+		close(0);
+
 		pack_pipe = fdopen(lp_pipe[1], "w");
 
 		if (create_full_pack)
@@ -167,6 +169,10 @@ static void create_pack_file(void)
 		const char *argv[10];
 		int i = 0;
 
+		close(0);
+		close(1);
+		close(2);
+
 		dup2(lp_pipe[0], 0);
 		dup2(pu_pipe[1], 1);
 		dup2(pe_pipe[1], 2);

Ciao,
Dscho

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

Johannes Schindelin wrote:
> 
> Something like this (totally untested):
> 
>  upload-pack.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index 498bf50..bafd90f 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -119,6 +119,8 @@ static void create_pack_file(void)
>  		int i;
>  		struct rev_info revs;
>  
> +		close(0);
> +
>  		pack_pipe = fdopen(lp_pipe[1], "w");
>  
>  		if (create_full_pack)

Shouldn't that be close(lp_pipe[0]);?

> @@ -167,6 +169,10 @@ static void create_pack_file(void)
>  		const char *argv[10];
>  		int i = 0;
>  
> +		close(0);
> +		close(1);
> +		close(2);
> +
>  		dup2(lp_pipe[0], 0);
>  		dup2(pu_pipe[1], 1);
>  		dup2(pe_pipe[1], 2);
> 

Those close()'s are redundant with the dup2's...

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

Junio C Hamano wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> ...  Going in with gdb, it appears the "timeout"
>> variable gets overwritten:
>>
>> (gdb) p timeout
>> $1 = 608471321
>>
>> ... which should have been 600.
> 
> I do not see offhand what can cause this.  The only new code
> since 1.4.4 series is "shallow clone" stuff,...

Well, we changed servers from an x86-64 box to an i386 box, so we're 
definitely running different binaries.

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

H. Peter Anvin wrote:
> git-1.5.0.3-1.i386 rpm from Junio's repository on kernel.org:
> 
> Since we got the new git server on kernel.org, we are having a problem 
> with git-upload-pack processes getting reparented to init, and then 
> sitting there forever.  Going in with gdb, it appears the "timeout" 
> variable gets overwritten:
> 
> (gdb) p timeout
> $1 = 608471321
> 
> ... which should have been 600.
> 
> The process spends effectively forever waiting in on the fflush() in 
> show_commit() (in upload-pack.c); /proc/*/fd shows it is trying to write 
> to a pipe, but I'm not sure what is at the other end of that same pipe.
> 

Another observation:

The value is always 608471321 (0x24448919) across all stuck processes 
that I've examined.  This is neither a recent time_t value (it would be 
Thu Apr 13 11:48:41 UTC 1989) nor is it a valid pointer value in any of 
the processes I've looked at.

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[H. Peter Anvin]

H. Peter Anvin wrote:
> Another observation:
> 
> The value is always 608471321 (0x24448919) across all stuck processes 
> that I've examined.  This is neither a recent time_t value (it would be 
> Thu Apr 13 11:48:41 UTC 1989) nor is it a valid pointer value in any of 
> the processes I've looked at.

Nevermind!  I just noticed that git-debuginfo RPM isn't from the same 
build as the git-core RPM, so anything obtained with gdb is baloney...

	-hpa

Re: git-upload-pack: the timeout gets corrupted?!

[Johannes Schindelin]

Hi,

On Sun, 11 Mar 2007, H. Peter Anvin wrote:

> Johannes Schindelin wrote:
> > 
> > Something like this (totally untested):
> > 
> >  upload-pack.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/upload-pack.c b/upload-pack.c
> > index 498bf50..bafd90f 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -119,6 +119,8 @@ static void create_pack_file(void)
> >  		int i;
> >  		struct rev_info revs;
> >  +		close(0);
> > +
> >  		pack_pipe = fdopen(lp_pipe[1], "w");
> >   		if (create_full_pack)
> 
> Shouldn't that be close(lp_pipe[0]);?

Yes, of course!

> > @@ -167,6 +169,10 @@ static void create_pack_file(void)
> >  		const char *argv[10];
> >  		int i = 0;
> >  +		close(0);
> > +		close(1);
> > +		close(2);
> > +
> >  		dup2(lp_pipe[0], 0);
> >  		dup2(pu_pipe[1], 1);
> >  		dup2(pe_pipe[1], 2);
> > 
> 
> Those close()'s are redundant with the dup2's...

I didn't know that, but that makes sense.

Ciao,
Dscho