From: Eric Wong <normalperson@...>
Date: 2014-02-21T07:31:54+00:00
Subject: [ruby-core:60928] Re: [ruby-trunk - Bug #9356] TCPSocket.new does not seem to handle INTR

shugo@ruby-lang.org wrote:
> Eric Wong wrote:
> >  Anyways, I have an alternative (v3) patch here which retries connect()
> >  on EINTR and ERESTART:
> >    http://bogomips.org/ruby.git/patch?id=8f48b1862
> >    (also note my new switch/case style to avoid inline ifdef :)
> >  
> >  However, I still prefer my v2 if possible:
> >    http://bogomips.org/ruby.git/patch?id=f5e2eb00e5
> 
> I prefer the latter too, but is `break` missing in the case clause?

I don't usually add it for the last case... Do some compilers need it?

> Linux's man page of connect(2) says it's conforming to POSIX.1-2001 (in spite of the fact that it returns ERESTART), so I hope we don't need the second connect().  What do you think, Kosaki-san?
> 
> Or only retry on ERESTART is enough.  We don't need the nested switch statements in the former patch, do we?

I'm not sure... Anyways, I think I may realize what happened with
Charlie's EINTR...

> And I'd like to add the following minimal error handling code to wait_connectable():

I think it is OK with a minor tweak below:

>     static int
>     wait_connectable(int fd)
>     {
>         int sockerr;
>         socklen_t sockerrlen;
>     
>         /*
>          * Stevens book says, successful finish turn on RB_WAITFD_OUT and
>          * failure finish turn on both RB_WAITFD_IN and RB_WAITFD_OUT.
>          * So it's enough to wait only RB_WAITFD_OUT and check the pending error
>          * by getsockopt().
>          *
>          * Note: rb_wait_for_single_fd already retries on EINTR/ERESTART
>          */
>         int revents = rb_wait_for_single_fd(fd, RB_WAITFD_OUT, NULL);
>     
>         if (revents < 0)
>     	    rb_bug_errno("rb_wait_for_single_fd used improperly", errno);
>     
>         sockerrlen = (socklen_t)sizeof(sockerr);
>         if (getsockopt(fd, SOL_SOCKET, SO_ERROR, (void *)&sockerr, &sockerrlen) < 0)
>             return -1;
>         if (sockerr != 0) {
>             errno = sockerr;

We should guard against sockerr setting errno to
EINTR, ERESTART, EINPROGRESS, EALREADY, EISCONN here...

Here is what I think happened in Charlie's original case:

	connect() -> EINTR, kernel socket remembers this!
	retry connect() -> EALREADY
	wait_connectable()
	    rb_wait_for_single_fd() -> success
	    getsockopt(SO_ERROR), sockerr = EINTR from first connect!
	    errno = sockerr -> raise :(