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

Yes I can confirm that Karl.
But to be sure if we undestand each other correctly, by it *becomes*
interruptable again *when* setting a timeout, I mean it becomes
killable (with a SIGKILL) again. so that means firing up another shell
and do "kill -9 <pid>".

And if we dont set a timeout it will result as Malcolm Rowe said: a
blocking connect which is not killable on OS X when you send a SIGINT
while its connecting.

I have made an proof of concept code for testing it on OS X, which
might be interesting to other OS X users/developers who want to test
it:

----
/* 
Unkillable process proof of concept code. Tested on Mac OS X 10.4.2. 
WARNING: If you run this you will most likely have to manually force a
hard reboot.
*/ 
#include <stdlib.h> 
#include <sys/types.h> 
#include <sys/socket.h> 
#include <netinet/in.h> 
#include <strings.h> 
#include <stdio.h> 
int main(void) 
{ 
  struct sockaddr_in server; 
  int sock, rc; 
  sock = socket( AF_INET, SOCK_STREAM, 0 ); 
  bzero(&server, sizeof(server)); 
  server.sin_family = AF_INET; 
  server.sin_port = htons(12345); 
  server.sin_addr.s_addr = inet_addr("www.google.com"); 
  printf("Try killing this process..\n"); 
  
  do { 
    rc = connect(sock, 
                 (const struct sockaddr *)&server, 
                 sizeof(server)); 
  } while (rc == -1); 
  return 1; 
}
-----
On 24 Aug 2005 22:24:03 -0500, kfogel@collab.net <kfogel@collab.net> wrote:
> 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