Re: [PATCHES] [patch] fe-connect.c doesn't handle EINTR correctly - Mailing list pgsql-hackers

Fix applied.  Thanks.

---------------------------------------------------------------------------

Bruce Momjian wrote:
>
> David, sorry you patch didn't make it into 7.2.X.  That whole EINTR
> discussion was quite complicated so I am not surprised we missed it.
>
> The attached patch implements your ENITR test in cases that seems to
> need it.  I have followed the method we used for ENITRY in fe-misc.c.
>
>
> ---------------------------------------------------------------------------
>
> David Ford wrote:
> > Last year we had a drawn out discussion about this and I created a patch
> > for it.  I never noticed that the patch didn't go in until I installed
> > 7.2 the other day and realised that fe-connect.c never was fixed.
> >
> > Here is the patch again.  It is against CVS 3/16/2002.  This time I only
> > rewrote the connect procedure at line 912, I leave it up to the regular
> > hackers to copy it's functionality to the SSL procedure just below it.
> >
> > In summary, if a software writer implements timer events or other events
> > which generate a signal with a timing fast enough to occur while libpq
> > is inside connect(), then connect returns -EINTR.  The code following
> > the connect call does not handle this and generates an error message.
> >  The sum result is that the pg_connect() fails.  If the timer or other
> > event is right on the window of the connect() completion time, the
> > pg_connect() may appear to work sporadically.  If the event is too slow,
> > pg_connect() will appear to always work and if the event is too fast,
> > pg_connect() will always fail.
> >
> > David
> >
>
> > Index: src/interfaces/libpq/fe-connect.c
> > ===================================================================
> > RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
> > retrieving revision 1.181
> > diff -u -r1.181 fe-connect.c
> > --- src/interfaces/libpq/fe-connect.c    2001/11/11 02:09:05    1.181
> > +++ src/interfaces/libpq/fe-connect.c    2002/03/16 05:17:47
> > @@ -909,29 +909,48 @@
> >       * Thus, we have to make arrangements for all eventualities.
> >       * ----------
> >       */
> > -    if (connect(conn->sock, &conn->raddr.sa, conn->raddr_len) < 0)
> > -    {
> > -        if (SOCK_ERRNO == EINPROGRESS || SOCK_ERRNO == EWOULDBLOCK || SOCK_ERRNO == 0)
> > -        {
> > -            /*
> > -             * This is fine - we're in non-blocking mode, and the
> > -             * connection is in progress.
> > -             */
> > -            conn->status = CONNECTION_STARTED;
> > -        }
> > -        else
> > -        {
> > -            /* Something's gone wrong */
> > -            connectFailureMessage(conn, SOCK_ERRNO);
> > -            goto connect_errReturn;
> > +    do {
> > +        int e;
> > +        e=connect(conn->sock, &conn->raddr.sa, conn->raddr_len)
> > +
> > +        if(e < 0) {
> > +            switch (e) {
> > +                case EINTR:
> > +                    /*
> > +                     * Interrupted by a signal, keep trying.  This handling is
> > +                     * required because the user may have turned on signals in
> > +                     * his program.  Previously, libpq would erronously fail to
> > +                     * connect if the user's timer event fired and interrupted
> > +                     * this syscall.  It is important that we don't try to sleep
> > +                     * here because this may cause havoc with the user program.
> > +                     */
> > +                    continue;
> > +                    break;
> > +                case 0:
> > +                case EINPROGRESS:
> > +                case EWOULDBLOCK:
> > +                    /*
> > +                     * This is fine - we're in non-blocking mode, and the
> > +                     * connection is in progress.
> > +                     */
> > +                    conn->status = CONNECTION_STARTED;
> > +                    break;
> > +                default:
> > +                    /* Something's gone wrong */
> > +                    connectFailureMessage(conn, SOCK_ERRNO);
> > +                    goto connect_errReturn;
> > +                    break;
> > +            }
> > +        } else {
> > +            /* We're connected now */
> > +            conn->status = CONNECTION_MADE;
> >          }
> > -    }
> > -    else
> > -    {
> > -        /* We're connected already */
> > -        conn->status = CONNECTION_MADE;
> > -    }
> > +
> > +        if(conn->status == CONNECTION_STARTED || conn->status == CONNECTION_MADE)
> > +            break;
> >
> > +    } while(1);
> > +
> >  #ifdef USE_SSL
> >      /* Attempt to negotiate SSL usage */
> >      if (conn->allow_ssl_try)
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 5: Have you checked our extensive FAQ?
> >
> > http://www.postgresql.org/users-lounge/docs/faq.html
>
> --
>   Bruce Momjian                        |  http://candle.pha.pa.us
>   pgman@candle.pha.pa.us               |  (610) 853-3000
>   +  If your life is a hard drive,     |  830 Blythe Avenue
>   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.182
> diff -c -r1.182 fe-connect.c
> *** src/interfaces/libpq/fe-connect.c    2 Mar 2002 00:49:22 -0000    1.182
> --- src/interfaces/libpq/fe-connect.c    14 Apr 2002 04:40:24 -0000
> ***************
> *** 913,920 ****
> --- 913,925 ----
>        * Thus, we have to make arrangements for all eventualities.
>        * ----------
>        */
> + retry1:
>       if (connect(conn->sock, &conn->raddr.sa, conn->raddr_len) < 0)
>       {
> +         if (SOCK_ERRNO == EINTR)
> +             /* Interrupted system call - we'll just try again */
> +             goto retry1;
> +
>           if (SOCK_ERRNO == EINPROGRESS || SOCK_ERRNO == EWOULDBLOCK || SOCK_ERRNO == 0)
>           {
>               /*
> ***************
> *** 949,957 ****
> --- 954,967 ----
>                                 SOCK_STRERROR(SOCK_ERRNO));
>               goto connect_errReturn;
>           }
> + retry2:
>           /* Now receive the postmasters response */
>           if (recv(conn->sock, &SSLok, 1, 0) != 1)
>           {
> +             if (SOCK_ERRNO == EINTR)
> +                 /* Interrupted system call - we'll just try again */
> +                 goto retry2;
> +
>               printfPQExpBuffer(&conn->errorMessage,
>                                 libpq_gettext("could not receive server response to SSL negotiation packet: %s\n"),
>                                 SOCK_STRERROR(SOCK_ERRNO));
> ***************
> *** 2132,2139 ****
> --- 2142,2153 ----
>                  "PQrequestCancel() -- socket() failed: ");
>           goto cancel_errReturn;
>       }
> + retry3:
>       if (connect(tmpsock, &conn->raddr.sa, conn->raddr_len) < 0)
>       {
> +         if (SOCK_ERRNO == EINTR)
> +             /* Interrupted system call - we'll just try again */
> +             goto retry3;
>           strcpy(conn->errorMessage.data,
>                  "PQrequestCancel() -- connect() failed: ");
>           goto cancel_errReturn;
> ***************
> *** 2150,2157 ****
> --- 2164,2175 ----
>       crp.cp.backendPID = htonl(conn->be_pid);
>       crp.cp.cancelAuthCode = htonl(conn->be_key);
>
> + retry4:
>       if (send(tmpsock, (char *) &crp, sizeof(crp), 0) != (int) sizeof(crp))
>       {
> +         if (SOCK_ERRNO == EINTR)
> +             /* Interrupted system call - we'll just try again */
> +             goto retry4;
>           strcpy(conn->errorMessage.data,
>                  "PQrequestCancel() -- send() failed: ");
>           goto cancel_errReturn;
> Index: src/interfaces/libpq/fe-misc.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v
> retrieving revision 1.68
> diff -c -r1.68 fe-misc.c
> *** src/interfaces/libpq/fe-misc.c    6 Mar 2002 06:10:42 -0000    1.68
> --- src/interfaces/libpq/fe-misc.c    14 Apr 2002 04:40:25 -0000
> ***************
> *** 361,367 ****
>       if (!conn || conn->sock < 0)
>           return -1;
>
> ! retry:
>       FD_ZERO(&input_mask);
>       FD_SET(conn->sock, &input_mask);
>       timeout.tv_sec = 0;
> --- 361,367 ----
>       if (!conn || conn->sock < 0)
>           return -1;
>
> ! retry1:
>       FD_ZERO(&input_mask);
>       FD_SET(conn->sock, &input_mask);
>       timeout.tv_sec = 0;
> ***************
> *** 371,377 ****
>       {
>           if (SOCK_ERRNO == EINTR)
>               /* Interrupted system call - we'll just try again */
> !             goto retry;
>
>           printfPQExpBuffer(&conn->errorMessage,
>                             libpq_gettext("select() failed: %s\n"),
> --- 371,377 ----
>       {
>           if (SOCK_ERRNO == EINTR)
>               /* Interrupted system call - we'll just try again */
> !             goto retry1;
>
>           printfPQExpBuffer(&conn->errorMessage,
>                             libpq_gettext("select() failed: %s\n"),
> ***************
> *** 395,401 ****
>       if (!conn || conn->sock < 0)
>           return -1;
>
> ! retry:
>       FD_ZERO(&input_mask);
>       FD_SET(conn->sock, &input_mask);
>       timeout.tv_sec = 0;
> --- 395,401 ----
>       if (!conn || conn->sock < 0)
>           return -1;
>
> ! retry2:
>       FD_ZERO(&input_mask);
>       FD_SET(conn->sock, &input_mask);
>       timeout.tv_sec = 0;
> ***************
> *** 405,411 ****
>       {
>           if (SOCK_ERRNO == EINTR)
>               /* Interrupted system call - we'll just try again */
> !             goto retry;
>
>           printfPQExpBuffer(&conn->errorMessage,
>                             libpq_gettext("select() failed: %s\n"),
> --- 405,411 ----
>       {
>           if (SOCK_ERRNO == EINTR)
>               /* Interrupted system call - we'll just try again */
> !             goto retry2;
>
>           printfPQExpBuffer(&conn->errorMessage,
>                             libpq_gettext("select() failed: %s\n"),
> ***************
> *** 478,484 ****
>       }
>
>       /* OK, try to read some data */
> ! tryAgain:
>   #ifdef USE_SSL
>       if (conn->ssl)
>           nread = SSL_read(conn->ssl, conn->inBuffer + conn->inEnd,
> --- 478,484 ----
>       }
>
>       /* OK, try to read some data */
> ! retry3:
>   #ifdef USE_SSL
>       if (conn->ssl)
>           nread = SSL_read(conn->ssl, conn->inBuffer + conn->inEnd,
> ***************
> *** 490,496 ****
>       if (nread < 0)
>       {
>           if (SOCK_ERRNO == EINTR)
> !             goto tryAgain;
>           /* Some systems return EAGAIN/EWOULDBLOCK for no data */
>   #ifdef EAGAIN
>           if (SOCK_ERRNO == EAGAIN)
> --- 490,496 ----
>       if (nread < 0)
>       {
>           if (SOCK_ERRNO == EINTR)
> !             goto retry3;
>           /* Some systems return EAGAIN/EWOULDBLOCK for no data */
>   #ifdef EAGAIN
>           if (SOCK_ERRNO == EAGAIN)
> ***************
> *** 531,537 ****
>               (conn->inBufSize - conn->inEnd) >= 8192)
>           {
>               someread = 1;
> !             goto tryAgain;
>           }
>           return 1;
>       }
> --- 531,537 ----
>               (conn->inBufSize - conn->inEnd) >= 8192)
>           {
>               someread = 1;
> !             goto retry3;
>           }
>           return 1;
>       }
> ***************
> *** 564,570 ****
>        * Still not sure that it's EOF, because some data could have just
>        * arrived.
>        */
> ! tryAgain2:
>   #ifdef USE_SSL
>       if (conn->ssl)
>           nread = SSL_read(conn->ssl, conn->inBuffer + conn->inEnd,
> --- 564,570 ----
>        * Still not sure that it's EOF, because some data could have just
>        * arrived.
>        */
> ! retry4:
>   #ifdef USE_SSL
>       if (conn->ssl)
>           nread = SSL_read(conn->ssl, conn->inBuffer + conn->inEnd,
> ***************
> *** 576,582 ****
>       if (nread < 0)
>       {
>           if (SOCK_ERRNO == EINTR)
> !             goto tryAgain2;
>           /* Some systems return EAGAIN/EWOULDBLOCK for no data */
>   #ifdef EAGAIN
>           if (SOCK_ERRNO == EAGAIN)
> --- 576,582 ----
>       if (nread < 0)
>       {
>           if (SOCK_ERRNO == EINTR)
> !             goto retry4;
>           /* Some systems return EAGAIN/EWOULDBLOCK for no data */
>   #ifdef EAGAIN
>           if (SOCK_ERRNO == EAGAIN)
> ***************
> *** 804,810 ****
>
>       if (forRead || forWrite)
>       {
> ! retry:
>           FD_ZERO(&input_mask);
>           FD_ZERO(&output_mask);
>           FD_ZERO(&except_mask);
> --- 804,810 ----
>
>       if (forRead || forWrite)
>       {
> ! retry5:
>           FD_ZERO(&input_mask);
>           FD_ZERO(&output_mask);
>           FD_ZERO(&except_mask);
> ***************
> *** 817,823 ****
>                      (struct timeval *) NULL) < 0)
>           {
>               if (SOCK_ERRNO == EINTR)
> !                 goto retry;
>               printfPQExpBuffer(&conn->errorMessage,
>                                 libpq_gettext("select() failed: %s\n"),
>                                 SOCK_STRERROR(SOCK_ERRNO));
> --- 817,823 ----
>                      (struct timeval *) NULL) < 0)
>           {
>               if (SOCK_ERRNO == EINTR)
> !                 goto retry5;
>               printfPQExpBuffer(&conn->errorMessage,
>                                 libpq_gettext("select() failed: %s\n"),
>                                 SOCK_STRERROR(SOCK_ERRNO));

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026