Re: Push hangs in latest msysgit 1.7.0.2
Peter Harris
>
> sideband_demux is needed to consume error message that the server side
> produces while pack_objects sends data to the server. By moving the demuxer
> later, there is nobody who consumes data (error messages) sent back from the
> server to the client; this could fill up network buffers and ultimately
> dead-lock the communication.
Dang. I missed that recv_sideband can write to stderr, and not just to
"out" (which has the limited buffer problem already, since the other
side of that pipe isn't read until receive_status is called).
> I'm afraid your patch is not a solution.
I've spent some more time (too much) looking at this.
It appears that a blocking read on a socket will prevent a write somewhere else.
A blocking select will also block a write somewhere else.
It appears a non-blocking select borks the socket, so that send-pack
dies when git tries to fstat(fileno(stdout)) in git.c (?!).
The docs for DuplicateHandle (which is what dup() uses under the hood)
have this to say:
"You should not use DuplicateHandle to duplicate handles to ...
Sockets. No error is returned, but the duplicate handle may not be
recognized by Winsock at the target process. Also, using
DuplicateHandle interferes with internal referene(sic) counting on the
underlying object. To duplicate a socket handle, use the
WSADuplicateSocket function."
I've looked at WSADuplicateSocket. It's ugly, and definitely not a
drop-in replacement for dup(), as it requires the receiving side to
cooperate.
I'm about ready to just disable sideband-64k.
Peter Harris
Johannes Sixt
> On Fri, Jun 18, 2010 at 5:53 PM, <msy...@googlecode.com> wrote:
> > sideband_demux is needed to consume error message that the server side
> > produces while pack_objects sends data to the server. By moving the
> > demuxer later, there is nobody who consumes data (error messages) sent
> > back from the server to the client; this could fill up network buffers
> > and ultimately dead-lock the communication.
>
> Dang. I missed that recv_sideband can write to stderr, and not just to
> "out" (which has the limited buffer problem already, since the other
> side of that pipe isn't read until receive_status is called).
But any data to be read from 'in' is only expected after the pack data has
been transfered, as per the definition of the protocol (at least I guess so,
I'm not an expert in this area). So there is no problem.
> It appears that a blocking read on a socket will prevent a write somewhere
> else.
>
> A blocking select will also block a write somewhere else.
>
> It appears a non-blocking select borks the socket, so that send-pack
> dies when git tries to fstat(fileno(stdout)) in git.c (?!).
It's mysterious. I didn't observe something like this. When I traced the
processes with Procmon, I found that the child hangs even before it can load
any DLLs. God knows what it's waiting for...
> The docs for DuplicateHandle (which is what dup() uses under the hood)
> have this to say:
> "You should not use DuplicateHandle to duplicate handles to ...
> Sockets. No error is returned, but the duplicate handle may not be
> recognized by Winsock at the target process. Also, using
> DuplicateHandle interferes with internal referene(sic) counting on the
> underlying object. To duplicate a socket handle, use the
> WSADuplicateSocket function."
>
> I've looked at WSADuplicateSocket. It's ugly, and definitely not a
> drop-in replacement for dup(), as it requires the receiving side to
> cooperate.
This paragraph is only about socket handles that are duplicated into another
process. But this is not what we want to happen: The child process
(pack-objects) only communicates via a pipe.
> I'm about ready to just disable sideband-64k.
I think this the way to go.
-- Hannes
Peter Harris
> On Mittwoch, 23. Juni 2010, Peter Harris wrote:
>> It appears that a blocking read on a socket will prevent a write somewhere
>> else.
>>
>> A blocking select will also block a write somewhere else.
>>
>> It appears a non-blocking select borks the socket, so that send-pack
>> dies when git tries to fstat(fileno(stdout)) in git.c (?!).
>
> It's mysterious. I didn't observe something like this. When I traced the
> processes with Procmon, I found that the child hangs even before it can load
> any DLLs. God knows what it's waiting for...
gdb was giving me more trouble than I care to admit, so I was working
with an MSVC build. Therefore I was using a newer libc.
Perhaps the older libc hangs at "alias fd 1 to stdout" time, rather
than just marking fd 1 invalid/closed? I haven't looked.
>> The docs for DuplicateHandle (which is what dup() uses under the hood)
>> have this to say:
>> "You should not use DuplicateHandle to duplicate handles to ...
>> Sockets. No error is returned, but the duplicate handle may not be
>> recognized by Winsock at the target process. Also, using
>> DuplicateHandle interferes with internal referene(sic) counting on the
>> underlying object. To duplicate a socket handle, use the
>> WSADuplicateSocket function."
>>
>> I've looked at WSADuplicateSocket. It's ugly, and definitely not a
>> drop-in replacement for dup(), as it requires the receiving side to
>> cooperate.
>
> This paragraph is only about socket handles that are duplicated into another
> process. But this is not what we want to happen: The child process
> (pack-objects) only communicates via a pipe.
If I read the source correctly, the handles (socket, and dup(socket))
created by git_connect are passed without modification to send_pack.
send_pack passes fd[1] to pack_objects without modification.
pack_objects passes fd into start_command (unless stateless_rpc is set).
Since fd >= 0, start_command does not create a pipe, but merely calls
dup() again.
Not that it makes a real difference as far as I can tell. I tried
creating a pipe (and another thread to forward data from the pipe to
the socket) and I had the same behaviour as the socket. There could
easily have been a bug in my attempt, of course.
>> I'm about ready to just disable sideband-64k.
>
> I think this the way to go.
Peter Harris