[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] fix non interruptable hang in svn client when connecting

From: <kfogel_at_collab.net>
Date: 2005-08-25 05:24:03 CEST

Yun Zheng Hu <yunzheng.hu@gmail.com> writes:
> [[[
> When you connect to a non existing repository, but the host exists,
> the apr_socket_connect will loop forever somehow, and is NOT
> interruptable.
> On OS X this will result in a inresponsive system, which is very bad.
>
> How to reproduce:
> svn co svn://svn.edgewall.com
>
> There is no svn server running on that port, so try a CTRL-C to
> cancel, but its not possible.. it will loop forever. process is not
> killable on OS X either.
>
> I found out that somebody else was having this problem too, but could
> not find a bug report and neither the fix. The user that was having
> this same bug can be read on:
>
> http://svn.haxx.se/users/archive-2005-07/1027.shtml
>
> The following patch will set a timeout of 10 seconds on apr_socket_connect.
> So if it cant connect it will timeout safely, 10 seconds should be
> more than enough I think.
> The patch will also include the port number in the error message, it
> might be useful or not..
> ]]]

Thanks for the patch. I have some technical comments below. Also,
there's no need to use the "[[[ ... ]]]" convention unless you are
actually putting a log message inside the braces. (You put the main
body of your message there instead.)

> The patch is very simple, open up:
> subversion/libsvn_ra_svn/client.c
>
> search for: "apr_socket_connect", and change the code to:
>
> status = apr_setsocketopt(*sock, APR_SO_TIMEOUT, 10 * APR_USEC_PER_SEC);
> if (status)
> return svn_error_wrap_apr(status, _("Can't set timeout"));
>
> status = apr_socket_connect(*sock, sa);
> if (status)
> return svn_error_wrap_apr(status, _("Can't connect to host '%s' on
> port %u"),
> hostname, port);
>

I think we shouldn't mix the port-number-in-error-message change with
the timeout change, as they are two unrelated things.

Also, do you know how to generate a real patch, that is, something
that can be applied using the 'patch' program? Just make your
modifications and run 'svn diff'. The result will be a patch, in the
format expected by all open source projects.

Now the technical notes:

apr_setsocketopt() is deprecated, and recommends the use of
apr_socket_set_opt() instead. However, if you look at the
documentation for apr_socket_set_opt(), the APR_SO_TIMEOUT option is
not listed as one of the valid options. Instead, one should use
apr_socket_set_timeout()! :-)

So that's what I've done in the patch below.

However, I'm not sure we should apply this patch; at least, I'd like
other developer's thoughts. There already seems to be a default
timeout, it's just long. It's around 3 minutes, according to my
tests:

   $ time svn co svn://svn.edgewall.com
   subversion/libsvn_ra_svn/client.c:141: (apr_err=110)
   svn: Can't connect to host 'svn.edgewall.com': Connection timed out
   
   real 3m9.548s
   user 0m0.020s
   sys 0m0.008s

   $ time svn co svn://svn.edgewall.com
   subversion/libsvn_ra_svn/client.c:141: (apr_err=110)
   svn: Can't connect to host 'svn.edgewall.com': Connection timed out
   
   real 3m9.056s
   user 0m0.022s
   sys 0m0.005s

   $

Now, if we lower that timeout, we're rejecting the default chosen by
APR, without doing anything to solve the root of the problem, which is
the lack of interruptability. On the other hand, the lack of
interruptability is sort of our fault, since we handle signals
ourselves. We have to run our check-cancellation function to actually
do anything with a signal, and that means we lose whenever we go into
an APR function like apr_socket_connect() :-(.

I'm not sure what the right answer is here. Should we lower the
socket timeout to 10 seconds, since we have made apr_socket_connect()
uninterruptable? Anyone have any thoughts?

Here's the new patch, anyway:

[[[
Fix a bug whereby connecting to a non-existent repository can cause
apr_socket_connect() to hang non-interruptably. To reproduce, try
'svn co svn://svn.edgewall.com'.

Patch by: Yun Zheng Hu <yunzheng.hu@gmail.com>
(Tweaked by me to use apr_socket_set_timeout instead of apr_setsocketopt.)

* subversion/libsvn_ra_svn/client.c
  (make_connection): Set socket timeout to 10 seconds.

Yun Zheng Hu's original message:

   http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=104587
   From: Yun Zheng Hu <yunzheng.hu@gmail.com>
   To: dev@subversion.tigris.org
   Subject: [PATCH] fix non interruptable hang in svn client when connecting
   Date: Thu, 25 Aug 2005 00:40:07 +0200
   Message-ID: <df84014e05082415405cdd9b13@mail.gmail.com>
]]]

Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c (revision 15898)
+++ subversion/libsvn_ra_svn/client.c (working copy)
@@ -136,6 +136,10 @@
   if (status)
     return svn_error_wrap_apr(status, _("Can't create socket"));
 
+ status = apr_socket_timeout_set(*sock, 10 * APR_USEC_PER_SEC);
+ if (status)
+ return svn_error_wrap_apr(status, _("Can't set timeout"));
+
   status = apr_socket_connect(*sock, sa);
   if (status)
     return svn_error_wrap_apr(status, _("Can't connect to host '%s'"),

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 25 06:23:41 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.