[PATCH] http client timeouts part1

Index: gwlib/http.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/http.c,v
retrieving revision 1.219
diff -a -u -p -r1.219 http.c
--- gwlib/http.c	11 Aug 2004 16:41:29 -0000	1.219
+++ gwlib/http.c	1 Nov 2004 22:25:48 -0000
@@ -601,6 +601,25 @@ static int entity_read(HTTPEntity *ent, 
  * HTTP client interface.
  */
 
+/*
+ * Internal lists of completely unhandled requests and requests for which
+ * a request has been sent but response has not yet been read.
+ */
+static List *pending_requests = NULL;
+
+
+/*
+ * Have background threads been started?
+ */
+static Mutex *client_thread_lock = NULL;
+static volatile sig_atomic_t client_threads_are_running = 0;
+
+
+/*
+ * Set of all connections to all servers. Used with conn_register to
+ * do I/O on several connections with a single thread.
+ */
+static FDSet *client_fdset = NULL;
 
 /*
  * Maximum number of HTTP redirections to follow. Making this infinite
@@ -629,7 +648,7 @@ typedef struct {
     List *request_headers;
     Octstr *request_body;   /* NULL for GET or HEAD, non-NULL for POST */
     enum {
-      connecting,
+	connecting,
 	request_not_sent,
 	reading_status,
 	reading_entity,
@@ -707,17 +726,13 @@ static void server_destroy(void *p)
  * Pool of open, but unused connections to servers or proxies. Key is
  * "servername:port", value is List with Connection objects.
  */
-static Dict *conn_pool = NULL;
-static Mutex *conn_pool_lock = NULL;
+static Dict *conn_pool;
+static Mutex *conn_pool_lock;
 
 
 static void conn_pool_item_destroy(void *item)
 {
-    Connection *conn;
-    
-    while ((conn = list_extract_first(item)) != NULL)
-    	conn_destroy(conn);
-    list_destroy(item, NULL);
+    list_destroy(item, (void(*)(void*))conn_destroy);
 }
 
 static void conn_pool_init(void)
@@ -734,7 +749,7 @@ static void conn_pool_shutdown(void)
 }
 
 
-static Octstr *conn_pool_key(Octstr *host, int port)
+static inline Octstr *conn_pool_key(Octstr *host, int port)
 {
     return octstr_format("%S:%d", host, port);
 }
@@ -754,15 +769,19 @@ static Connection *conn_pool_get(Octstr 
     if (list == NULL)
     	conn = NULL;
     else {
-	while (1) {
-	    conn = list_extract_first(list);
-	    if (conn == NULL)
-		break;
-	    /* Check whether the server has closed the connection while
-	     * it has been in the pool. */
-	    conn_wait(conn, 0);
-	    if (!conn_eof(conn) && !conn_error(conn))
-		break;
+        while ((conn = list_extract_first(list)) != NULL) {
+            /*
+             * Check whether the server has closed the connection while
+             * it has been in the pool.
+             */
+            conn_wait(conn, 0);
+            if (!conn_eof(conn) && !conn_error(conn)) {
+#ifdef USE_KEEPALIVE
+                /* unregister our server disconnect callback */
+                conn_unregister(conn);
+#endif 
+                break;
+            }
 	    conn_destroy(conn);
 	}
     }
@@ -786,6 +805,40 @@ static Connection *conn_pool_get(Octstr 
 }
 
 #ifdef USE_KEEPALIVE
+static void check_pool_conn(Connection *conn, void *data)
+{
+    Octstr *key = data;
+    if (run_status != running) {
+        octstr_destroy(key);
+        conn_unregister(conn);
+        return;
+    }
+    /* check if connection still ok */
+    conn_wait(conn, 0);
+    if (conn_error(conn) || conn_eof(conn)) {
+        List *list;
+        mutex_lock(conn_pool_lock);
+        list = dict_get(conn_pool, key);
+        if (list_delete_equal(list, conn) > 0) {
+            /*
+             * ok, connection was still within pool. So it's
+             * safe to destroy this connection.
+             */
+            conn_destroy(conn);
+            octstr_destroy(key);
+        } else {
+            /* hmm, bad... connections is already used */
+            error(0, "HTTP: Race condition detected: Could not found connection in pool!");
+            /*
+             * don't unregister or destroy our key.
+             * Better memleak as segfault ;)
+             */
+        }
+        mutex_unlock(conn_pool_lock);
+    }
+}
+
+
 static void conn_pool_put(Connection *conn, Octstr *host, int port)
 {
     Octstr *key;
@@ -799,33 +852,14 @@ static void conn_pool_put(Connection *co
         dict_put(conn_pool, key, list);
     }
     list_append(list, conn);
+    /* register connection to get server disconnect */
+    conn_register(conn, client_fdset, check_pool_conn, key);
     octstr_destroy(key);
     mutex_unlock(conn_pool_lock);
 }
 #endif
 
 
-/*
- * Internal lists of completely unhandled requests and requests for which
- * a request has been sent but response has not yet been read.
- */
-static List *pending_requests = NULL;
-
-
-/*
- * Have background threads been started?
- */
-static Mutex *client_thread_lock = NULL;
-static volatile sig_atomic_t client_threads_are_running = 0;
-
-
-/*
- * Set of all connections to all servers. Used with conn_register to
- * do I/O on several connections with a single thread.
- */
-static FDSet *client_fdset = NULL;
-
-
 HTTPCaller *http_caller_create(void)
 {
     HTTPCaller *caller;
@@ -1025,7 +1059,7 @@ static void handle_transaction(Connectio
 
 #ifdef DUMP_RESPONSE
                 /* Dump the response */
-                debug("wsp.http", 0, "HTTP: Received response:");
+                debug("gwlib.http", 0, "HTTP: Received response:");
                 h = build_response(trans->response->headers, trans->response->body);
                 octstr_dump(h, 0);
                 octstr_destroy(h);
@@ -1051,11 +1085,11 @@ static void handle_transaction(Connectio
     if (trans->persistent) {
         if (proxy_used_for_host(trans->host))
             conn_pool_put(trans->conn, proxy_hostname, proxy_port);
-        else
+        else 
             conn_pool_put(trans->conn, trans->host, trans->port);
     } else
 #endif
-    	conn_destroy(trans->conn);
+        conn_destroy(trans->conn);
 
     trans->conn = NULL;
 
@@ -1422,13 +1456,11 @@ static void parse2trans(HTTPURLParse *p,
 
 static Connection *get_connection(HTTPServer *trans) 
 {
-    Connection *conn;
+    Connection *conn = NULL;
     Octstr *host;
     HTTPURLParse *p;
     int port;
 
-    conn = NULL;
-
     /* if the parsing has not yet been done, then do it now */
     if (!trans->host && trans->port == 0 && trans->url != NULL) {
         if ((p = parse_url(trans->url)) != NULL) {
@@ -1447,26 +1479,17 @@ static Connection *get_connection(HTTPSe
         port = trans->port;
     }
 
-    if (trans->retrying) {
-#ifdef HAVE_LIBSSL
-    if (trans->ssl) conn = conn_open_ssl(host, port, trans->certkeyfile, http_interface);
-        else
-#endif /* HAVE_LIBSSL */
-      conn = conn_open_tcp_nb(host, port, http_interface);
-            debug("gwlib.http", 0, "HTTP: Opening NEW connection to `%s:%d' (fd=%d).",
-                  octstr_get_cstr(host), port, conn_get_id(conn));
-    } else
     conn = conn_pool_get(host, port, trans->ssl, trans->certkeyfile,
                          http_interface);
     if (conn == NULL)
         goto error;
 
-  return conn;
+    return conn;
 
- error:
-  conn_destroy(conn);
-  error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
-  return NULL;
+error:
+    conn_destroy(conn);
+    error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
+    return NULL;
 }
 
 
@@ -1476,47 +1499,47 @@ static Connection *get_connection(HTTPSe
  */
 static int send_request(HTTPServer *trans)
 {
-  Octstr *request;
+    Octstr *request;
 
-  request = NULL;
+    request = NULL;
 
-  /* 
-   * we have to assume all values in trans are already set
-   * by parse_url() before calling this.
-   */
-
-  if (trans->username != NULL)
-    http_add_basic_auth(trans->request_headers, trans->username,
-			trans->password);
-
-  if (proxy_used_for_host(trans->host)) {
-    proxy_add_authentication(trans->request_headers);
-    request = build_request(http_method2name(trans->method),
-			    trans->url, trans->host, trans->port, 
-			    trans->request_headers, 
-			    trans->request_body);
-  } else {
-    request = build_request(http_method2name(trans->method), trans->uri, 
-			    trans->host, trans->port,
-			    trans->request_headers,
-			    trans->request_body);
-  }
+    /* 
+    * we have to assume all values in trans are already set
+    * by parse_url() before calling this.
+    */
+
+    if (trans->username != NULL)
+        http_add_basic_auth(trans->request_headers, trans->username,
+                            trans->password);
+
+    if (proxy_used_for_host(trans->host)) {
+        proxy_add_authentication(trans->request_headers);
+        request = build_request(http_method2name(trans->method),
+                                trans->url, trans->host, trans->port, 
+                                trans->request_headers, 
+                                trans->request_body);
+    } else {
+        request = build_request(http_method2name(trans->method), trans->uri, 
+                                trans->host, trans->port,
+                                trans->request_headers,
+                                trans->request_body);
+    }
   
-    debug("wsp.http", 0, "HTTP: Sending request:");
+    debug("gwlib.http", 0, "HTTP: Sending request:");
     octstr_dump(request, 0);
-  if (conn_write(trans->conn, request) == -1)
+    if (conn_write(trans->conn, request) == -1)
         goto error;
 
     octstr_destroy(request);
 
-  return 0;
+    return 0;
 
- error:
-  conn_destroy(trans->conn);
-  trans->conn = NULL;
+error:
+    conn_destroy(trans->conn);
+    trans->conn = NULL;
     octstr_destroy(request);
     error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
-  return -1;
+    return -1;
 }
 
 
@@ -1544,46 +1567,43 @@ static void write_request_thread(void *a
          */
         trans->conn = get_connection(trans);
 
-	if (trans->conn == NULL)
-	  list_produce(trans->caller, trans);
-        else {
-          if (conn_is_connected(trans->conn) == 0) {
-	    debug("gwlib.http", 0, "Socket connected at once");
-
-        if (trans->method == HTTP_METHOD_POST) {
-            /* 
-             * Add a Content-Length header.  Override an existing one, if
-             * necessary.  We must have an accurate one in order to use the
-             * connection for more than a single request.
-             */
-            http_header_remove_all(trans->request_headers, "Content-Length");
-            sprintf(buf, "%ld", octstr_len(trans->request_body));
-            http_header_add(trans->request_headers, "Content-Length", buf);
-        } 
+        if (trans->conn == NULL)
+            list_produce(trans->caller, trans);
+        else if (conn_is_connected(trans->conn) == 0) {
+            debug("gwlib.http", 0, "Socket connected at once");
+
+            if (trans->method == HTTP_METHOD_POST) {
+                /* 
+                * Add a Content-Length header.  Override an existing one, if
+                * necessary.  We must have an accurate one in order to use the
+                * connection for more than a single request.
+                */
+                http_header_remove_all(trans->request_headers, "Content-Length");
+                sprintf(buf, "%ld", octstr_len(trans->request_body));
+                http_header_add(trans->request_headers, "Content-Length", buf);
+            } 
             /* 
              * ok, this has to be an GET or HEAD request method then,
              * if it contains a body, then this is not HTTP conform, so at
              * least warn the user 
              */
-        else if (trans->request_body != NULL) {
-            warning(0, "HTTP: GET or HEAD method request contains body:");
-            octstr_dump(trans->request_body, 0);
-        }
-	    if ((rc = send_request(trans)) == 0) {
-            trans->state = reading_status;
-              conn_register(trans->conn, client_fdset, handle_transaction, 
-                            trans);
+            else if (trans->request_body != NULL) {
+                warning(0, "HTTP: GET or HEAD method request contains body:");
+                octstr_dump(trans->request_body, 0);
+            }
+            if ((rc = send_request(trans)) == 0) {
+                trans->state = reading_status;
+                conn_register(trans->conn, client_fdset, handle_transaction, 
+                                trans);
             } else {
-              list_produce(trans->caller, trans);
+                list_produce(trans->caller, trans);
             }
 
-          } else { /* Socket not connected, wait for connection */
+        } else { /* Socket not connected, wait for connection */
             debug("gwlib.http", 0, "Socket connecting");
             trans->state = connecting;
             conn_register(trans->conn, client_fdset, handle_transaction, trans);
         }
-	  
-	}
     }
 }
 
@@ -1609,7 +1629,7 @@ static void start_client_threads(void)
 
 void http_set_interface(const Octstr *our_host)
 {
-  http_interface = octstr_duplicate(our_host);
+    http_interface = octstr_duplicate(our_host);
 }
 
 
@@ -2656,7 +2676,7 @@ Octstr *http_header_value(List *headers,
 List *http_header_duplicate(List *headers)
 {
     List *new;
-    long i;
+    long i, len;
 
     gwlib_assert_init();
 
@@ -2664,7 +2684,8 @@ List *http_header_duplicate(List *header
         return NULL;
 
     new = http_create_empty_headers();
-    for (i = 0; i < list_len(headers); ++i)
+    len = list_len(headers);
+    for (i = 0; i < len; ++i)
         list_append(new, octstr_duplicate(list_get(headers, i)));
     return new;
 }