Code review for connection timeout patch. Avoid unportable assumption

that tv_sec is signed; return a useful error message on timeout failure;
honor PGCONNECT_TIMEOUT environment variable in PQsetdbLogin; make code
obey documentation statement that timeout=0 means no timeout.

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 65775de9d64456215b2492e087ed408d091be7f3..7cdd2466624469daecb16563fb419eb4d6972b52 100644 (file)
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v 1.212 2002/10/16 04:38:00 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v 1.213 2002/10/24 23:35:55 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -507,6 +507,9 @@ PQsetdbLogin(const char *pghost, const char *pgport, const char *pgoptions,
        else
                conn->pgpass = strdup(DefaultPassword);
 
+       if ((tmp = getenv("PGCONNECT_TIMEOUT")) != NULL)
+               conn->connect_timeout = strdup(tmp);
+
 #ifdef USE_SSL
        if ((tmp = getenv("PGREQUIRESSL")) != NULL)
                conn->require_ssl = (tmp[0] == '1') ? true : false;
@@ -1051,32 +1054,29 @@ static int
 connectDBComplete(PGconn *conn)
 {
        PostgresPollingStatusType flag = PGRES_POLLING_WRITING;
-
-       time_t                  finish_time = -1;
+       time_t                  finish_time = ((time_t) -1);
 
        if (conn == NULL || conn->status == CONNECTION_BAD)
                return 0;
 
        /*
-        * Prepare to time calculations, if connect_timeout isn't zero.
+        * Set up a time limit, if connect_timeout isn't zero.
         */
        if (conn->connect_timeout != NULL)
        {
                int timeout = atoi(conn->connect_timeout);
 
-               if (timeout == 0)
+               if (timeout > 0)
                {
-                       conn->status = CONNECTION_BAD;
-                       return 0;
+                       /* Rounding could cause connection to fail; need at least 2 secs */
+                       if (timeout < 2)
+                               timeout = 2;
+                       /* calculate the finish time based on start + timeout */
+                       finish_time = time(NULL) + timeout;
                }
-               /* Rounding could cause connection to fail;we need at least 2 secs */
-               if (timeout == 1)
-                       timeout++;
-               /* calculate the finish time based on start + timeout */
-               finish_time = time(NULL) + timeout;
        }
 
-       while (finish_time == -1 || time(NULL) < finish_time)
+       for (;;)
        {
                /*
                 * Wait, if necessary.  Note that the initial state (just after
@@ -1118,8 +1118,6 @@ connectDBComplete(PGconn *conn)
                 */
                flag = PQconnectPoll(conn);
        }
-       conn->status = CONNECTION_BAD;
-       return 0;
 }
 
 /* ----------------

diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 6cb7013514eb302b169dc6a883989a85261e049b..f94f46dd59bc21aef8b0152a55d56339d67cf5c4 100644 (file)
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -25,7 +25,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v 1.84 2002/10/16 02:55:30 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/interfaces/libpq/fe-misc.c,v 1.85 2002/10/24 23:35:55 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -779,18 +779,27 @@ pqFlush(PGconn *conn)
 int
 pqWait(int forRead, int forWrite, PGconn *conn)
 {
-       return pqWaitTimed(forRead, forWrite, conn, -1);
+       return pqWaitTimed(forRead, forWrite, conn, (time_t) -1);
 }
 
+/*
+ * pqWaitTimed: wait, but not past finish_time.
+ *
+ * If finish_time is exceeded then we return failure (EOF).  This is different
+ * from the response for a kernel exception (return 0) because we don't want
+ * the caller to try to read/write in that case.
+ *
+ * finish_time = ((time_t) -1) disables the wait limit.
+ */
 int
 pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
 {
        fd_set          input_mask;
        fd_set          output_mask;
        fd_set          except_mask;
-
        struct timeval tmp_timeout;
        struct timeval *ptmp_timeout = NULL;
+       int                     selresult;
 
        if (conn->sock < 0)
        {
@@ -820,24 +829,26 @@ retry5:
                        FD_SET(conn->sock, &output_mask);
                FD_SET(conn->sock, &except_mask);
 
-               if (finish_time != -1)
+               if (finish_time != ((time_t) -1))
                {
                        /*
-                        *      select() may modify timeout argument on some platforms so
-                        *      use copy.
-                        *      XXX Do we really want to do that?  If select() returns
-                        *      the number of seconds remaining, we are resetting
-                        *      the timeout to its original value.  This will yeild
-                        *      incorrect timings when select() is interrupted.
-                        *      bjm 2002-10-14
+                        * Set up delay.  Assume caller incremented finish_time
+                        * so that we can error out as soon as time() passes it.
+                        * Note we will recalculate delay each time through the loop.
                         */
-                       if ((tmp_timeout.tv_sec = finish_time - time(NULL)) < 0)
-                               tmp_timeout.tv_sec = 0;  /* possible? */
+                       time_t  now = time(NULL);
+
+                       if (finish_time > now)
+                               tmp_timeout.tv_sec = finish_time - now;
+                       else
+                               tmp_timeout.tv_sec = 0;
                        tmp_timeout.tv_usec = 0;
                        ptmp_timeout = &tmp_timeout;
                }
-               if (select(conn->sock + 1, &input_mask, &output_mask,
-                                  &except_mask, ptmp_timeout) < 0)
+
+               selresult = select(conn->sock + 1, &input_mask, &output_mask,
+                                                  &except_mask, ptmp_timeout);
+               if (selresult < 0)
                {
                        if (SOCK_ERRNO == EINTR)
                                goto retry5;
@@ -846,6 +857,12 @@ retry5:
                                                          SOCK_STRERROR(SOCK_ERRNO));
                        return EOF;
                }
+               if (selresult == 0)
+               {
+                       printfPQExpBuffer(&conn->errorMessage,
+                                                         libpq_gettext("timeout expired\n"));
+                       return EOF;
+               }
        }
 
        return 0;