On Wed, May 27, 2009 at 12:50:59PM -0400, Greg Hudson wrote:
> I looked over the code, and I believe you seriously broke things in
> r37838, without improving anything related to the issue.
Ooops! Too bad the regression tests we have didn't catch this... :(
> The default timeout for reading from an APR (or Unix) socket is
> infinity. During pipelining, we set the timeout to 0 for some write
> opreations so that we can try them without blocking. If APR had
> separate timeouts for read and write, I would have only set the write
> timeout in the first place, and that code in sock_read_cb would be
> unnecessary. The comment could perhaps have explained this better, but
> I'm not sure if it could have explained it well enough to dispel your
> assumptions about what the code was doing.
Right, that clears up the mystery. Thank you!
> What you did was remove the code which reverts back to blocking, leaving
> the timeout at 0 in certain situations. I don't know exactly what
> symptoms this would cause, but I imagine they're nothing good.
Yeah that's bad. I'll revert.
> As a matter of history, I've not been impressed with application-level
> hardcoded timeouts, so I didn't put any into ra_svn. It's not the case,
> as you said in #3347, that I explicitly removed some default timeout
> present in APR or the operating system; those layers don't have default
> timeouts.
>
> What I would recommend to address #3347 is setting the SO_KEEPALIVE flag
> on the socket, which will cause the underlying TCP layer to use
> keepalive packets to eventually notice when the other half of the
> network connection has disappeared without sending a reset. I'm not
> sure how to do that through APR, but it's probably not too difficult.
Thanks for the hint. I'll try looking into this, too see if it is
possible to fix this properly.
> As a matter of process, you should never assume that a piece of code is
> present for no reason when you have the means to investigate why it's
> there. Using svn blame, you could have (after tracing back through the
> code movement in r22238) seen that the code was introduced in r7532, and
> correctly interpreted it as part of the pipelining support.
Well, I used blame, and log, to try to find out what the heck was going on.
I ended up assuming that the code was introduced as part of streams.c when
it was created. Below is the log message for r22238. Please let me know
if you can find anything in it that indicates that the code in question
was moved there from elsewhere, or even that it was written by you.
Because I cannot find anything. I only found out that you wrote the
code because Erik pointed it out in this thread.
The only thing I didn't look at was the diff of that revision.
That may of course have revealed some information.
But if someone just says "New file" in a log message without giving
further information, then I can rightly assume that the file is in
fact new and does not contain anything from elsewhere.
Moving symbols across files is not "New file". Part of moving symbols
is mentioning their old location.
So, sorry about that. I tried my best to follow process, but sometimes
that alone is just not enough. I'm glad you wrote your email, because
otherwise I would have had to find out the hard way that my "fix" was
not a fix at all.
------------------------------------------------------------------------
r22238 | rooneg | 2006-11-08 23:07:54 +0000 (Wed, 08 Nov 2006) | 30 lines
Encapsulate ra_svn's I/O with a stream-based wrapper. This will
facilitate the introduction of SASL and TLS encryption.
Patch by: Vlad Georgescu <vgeorgescu_at_gmail.com>
* subversion/libsvn_ra_svn/marshal.c:
Update the copyright date.
(svn_ra_svn_create_conn): Create the connection stream. Don't initialize
in_file and out_file.
(svn_ra_svn__set_block_handler, svn_ra_svn__input_waiting,
writebuf_output, readbuf_input): Use the new svn_ra_svn__stream_t interface
instead of the old apr_file_t/apr_socket_t code.
* subversion/libsvn_ra_svn/ra_svn.h
(ra_svn_pending_fn_t,
ra_svn_timeout_fn_t,
svn_ra_svn__stream_t): New typedefs.
(svn_ra_svn_conn_st): Add stream. Remove in_file, out_file and proc. Explain
that direct access to sock is still required by SASL.
(svn_ra_svn__stream_from_sock,
svn_ra_svn__stream_from_files,
svn_ra_svn__stream_create,
svn_ra_svn__stream_write,
svn_ra_svn__stream_read,
svn_ra_svn__stream_timeout,
svn_ra_svn__stream_pending): New function declarations.
* subversion/libsvn_ra_svn/streams.c: New file. Implements the
svn_ra_svn__stream_t interface for socket and file streams.
------------------------------------------------------------------------
Stefan
Received on 2009-05-27 19:21:22 CEST