prevent runaway connections when server hangs up prematurely

panda/src/downloader/bioStreamBuf.cxx

@@ -165,11 +165,18 @@ underflow() {
       if (read_count <= 0) {
         _is_closed = !BIO_should_retry(*_source);
         if (_is_closed) {
-          downloader_cat.info()
-            << "Socket error detected on " 
-            << _source->get_server_name() << ":" 
-            << _source->get_port() << ", return code "
-            << read_count << ".\n";
+          if (read_count == 0) {
+            downloader_cat.info()
+              << "Lost connection to "
+              << _source->get_server_name() << ":" 
+              << _source->get_port() << ".\n";
+          } else {
+            downloader_cat.info()
+              << "Socket error detected on " 
+              << _source->get_server_name() << ":" 
+              << _source->get_port() << ", return code "
+              << read_count << ".\n";
+          }
 #ifdef REPORT_OPENSSL_ERRORS
           ERR_print_errors_fp(stderr);
 #endif

panda/src/downloader/config_downloader.cxx

@@ -85,11 +85,26 @@ config_downloader.GetString("http-proxy", "");
 const string http_proxy_username =
 config_downloader.GetString("http-proxy-username", "");
 
-// This is the default amount of time to wait for a connection, in
-// seconds.  It is presently only used for nonblocking sockets.
+// This is the default amount of time to wait for a TCP/IP connection
+// to be established, in seconds.  It is presently only used for
+// nonblocking sockets.
 const double connect_timeout =
 config_downloader.GetDouble("connect-timeout", 5.0);
 
+// This is the default amount of time to wait for the HTTP server to
+// finish sending its response to our request, in seconds.  It starts
+// counting after the TCP connection has been established
+// (connect_timeout, above) and the request has been sent.
+const double http_timeout =
+config_downloader.GetDouble("http-timeout", 20.0);
+
+// This is the maximum number of times to try reconnecting to the
+// server on any one document attempt.  This is just a failsafe to
+// prevent the code from attempting runaway connections; this limit
+// should never be reached in practice.
+const int http_max_connect_count =
+config_downloader.GetInt("http-max-connect-count", 10);
+
 ConfigureFn(config_downloader) {
 #ifdef HAVE_SSL
   HTTPChannel::init_type();

panda/src/downloader/config_downloader.h

@@ -45,6 +45,8 @@ extern const bool verify_ssl;
 extern const string http_proxy;
 extern const string http_proxy_username;
 extern const double connect_timeout;
+extern const double http_timeout;
+extern const int http_max_connect_count;
 
 // Later, we can make this conditional on NDEBUG or something along
 // those lines; for now, we define it always to be true so we get

panda/src/downloader/httpChannel.I

@@ -200,7 +200,7 @@ get_persistent_connection() const {
 //               channel will wait before giving up on establishing a
 //               TCP connection.
 //
-//               At the present, this is used only for the nonblocking
+//               At present, this is used only for the nonblocking
 //               interfaces (e.g. begin_get_document(),
 //               begin_connect_to()), but it is used whether
 //               set_blocking_connect() is true or false.
@@ -231,6 +231,14 @@ get_connect_timeout() const {
 //               false, a socket connect will not block for
 //               nonblocking I/O calls, but will block for blocking
 //               I/O calls (get_document(), connect_to(), etc.).
+
+//               Setting this true is useful when you want to use
+//               non-blocking I/O once you have established the
+//               connection, but you don't want to bother with polling
+//               for the initial connection.  It's also useful when
+//               you don't particularly care about non-blocking I/O,
+//               but you need to respect timeouts like connect_timeout
+//               and http_timeout.
 ////////////////////////////////////////////////////////////////////
 INLINE void HTTPChannel::
 set_blocking_connect(bool blocking_connect) {
@@ -252,6 +260,39 @@ get_blocking_connect() const {
   return _blocking_connect;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: HTTPChannel::set_http_timeout
+//       Access: Published
+//  Description: Sets the maximum length of time, in seconds, that the
+//               channel will wait for the HTTP server to finish
+//               sending its response to our request.
+//
+//               The timer starts counting after the TCP connection
+//               has been established (see set_connect_timeout(),
+//               above) and the request has been sent.
+//
+//               At present, this is used only for the nonblocking
+//               interfaces (e.g. begin_get_document(),
+//               begin_connect_to()), but it is used whether
+//               set_blocking_connect() is true or false.
+////////////////////////////////////////////////////////////////////
+INLINE void HTTPChannel::
+set_http_timeout(double http_timeout) {
+  _http_timeout = http_timeout;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: HTTPChannel::get_http_timeout
+//       Access: Published
+//  Description: Returns the length of time, in seconds, to wait for 
+//               the HTTP server to respond to our request.  See
+//               set_http_timeout().
+////////////////////////////////////////////////////////////////////
+INLINE double HTTPChannel::
+get_http_timeout() const {
+  return _http_timeout;
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: HTTPChannel::set_download_throttle
 //       Access: Published

panda/src/downloader/httpChannel.cxx

@@ -43,6 +43,7 @@ HTTPChannel(HTTPClient *client) :
 {
   _persistent_connection = false;
   _connect_timeout = connect_timeout;
+  _http_timeout = http_timeout;
   _blocking_connect = false;
   _download_throttle = false;
   _max_bytes_per_second = downloader_byte_rate;
@@ -254,6 +255,17 @@ run() {
   bool repeat_later;
   do {
     if (_bio.is_null()) {
+      if (_connect_count > http_max_connect_count) {
+        // Too many connection attempts, just give up.  We should
+        // never trigger this failsafe, since the code in each
+        // individual case has similar logic to prevent more than two
+        // consecutive lost connections.
+        downloader_cat.warning()
+          << "Too many lost connections, giving up.\n";
+        _state = S_failure;
+        return false;
+      }
+
       // No connection.  Attempt to establish one.
       _proxy = _client->get_proxy();
       
@@ -266,10 +278,17 @@ run() {
       if (_nonblocking) {
         BIO_set_nbio(*_bio, 1);
       }
+
+      if (_connect_count > 0) {
+        downloader_cat.info()
+          << "Reconnecting to " << _bio->get_server_name() << ":" 
+          << _bio->get_port() << "\n";
+      }
       
       _state = S_connecting;
       _started_connecting_time = 
         ClockObject::get_global_clock()->get_real_time();
+      _connect_count++;
     }
 
     if (downloader_cat.is_spam()) {
@@ -667,8 +686,7 @@ run_connecting_wait() {
 
   if (downloader_cat.is_debug()) {
     downloader_cat.debug()
-      << "waiting to connect to " << _url.get_server() << ":" 
-      << _url.get_port() << ".\n";
+      << "waiting to connect to " << _url.get_server_and_port() << ".\n";
   }
   fd_set wset;
   FD_ZERO(&wset);
@@ -699,8 +717,7 @@ run_connecting_wait() {
          _started_connecting_time > get_connect_timeout())) {
       // Time to give up.
       downloader_cat.info()
-        << "Timeout connecting to " << _url.get_server() << ":" 
-        << _url.get_port() << ".\n";
+        << "Timeout connecting to " << _url.get_server_and_port() << ".\n";
       _state = S_failure;
       return false;
     }
@@ -877,7 +894,7 @@ run_ssl_handshake() {
     }
     downloader_cat.info()
       << "Could not establish SSL handshake with " 
-      << _url.get_server() << ":" << _url.get_port() << "\n";
+      << _url.get_server_and_port() << "\n";
 #ifdef REPORT_OPENSSL_ERRORS
     ERR_print_errors_fp(stderr);
 #endif
@@ -904,8 +921,7 @@ run_ssl_handshake() {
   long verify_result = SSL_get_verify_result(ssl);
   if (verify_result == X509_V_ERR_CERT_HAS_EXPIRED) {
     downloader_cat.info()
-      << "Expired certificate from " << _url.get_server() << ":"
-      << _url.get_port() << "\n";
+      << "Expired certificate from " << _url.get_server_and_port() << "\n";
     if (_client->get_verify_ssl() == HTTPClient::VS_normal) {
       _state = S_failure;
       return false;
@@ -913,8 +929,7 @@ run_ssl_handshake() {
 
   } else if (verify_result == X509_V_ERR_CERT_NOT_YET_VALID) {
     downloader_cat.info()
-      << "Premature certificate from " << _url.get_server() << ":"
-      << _url.get_port() << "\n";
+      << "Premature certificate from " << _url.get_server_and_port() << "\n";
     if (_client->get_verify_ssl() == HTTPClient::VS_normal) {
       _state = S_failure;
       return false;
@@ -922,8 +937,8 @@ run_ssl_handshake() {
 
   } else if (verify_result != X509_V_OK) {
     downloader_cat.info()
-      << "Unable to verify identity of " << _url.get_server() << ":" 
-      << _url.get_port() << ", verify error code " << verify_result << "\n";
+      << "Unable to verify identity of " << _url.get_server_and_port()
+      << ", verify error code " << verify_result << "\n";
     if (_client->get_verify_ssl() != HTTPClient::VS_no_verify) {
       _state = S_failure;
       return false;
@@ -997,6 +1012,7 @@ run_ready() {
     
   // All done sending request.
   _state = S_request_sent;
+  _sent_request_time = ClockObject::get_global_clock()->get_real_time();
   return false;
 }
 
@@ -1021,18 +1037,23 @@ run_request_sent() {
         // Try again, once.
         _response_type = RT_hangup;
       }
+
+    } else if (ClockObject::get_global_clock()->get_real_time() -
+               _sent_request_time > get_http_timeout()) {
+      // Time to give up.
+      downloader_cat.info()
+        << "Timeout waiting for " << _url.get_server_and_port() << ".\n";
+      _state = S_failure;
     }
+
     return true;
   }
 
   if (!parse_http_response(line)) {
+    // Not an HTTP response.  _state is already set appropriately.
     return false;
   }
 
-  // Ok, we've established an HTTP connection to the server.  Our
-  // extra send headers have done their job; clear them for next time.
-  clear_extra_headers();
-
   _state = S_reading_header;
   _current_field_name = string();
   _current_field_value = string();
@@ -1052,8 +1073,32 @@ run_request_sent() {
 bool HTTPChannel::
 run_reading_header() {
   if (parse_http_header()) {
+    if (_bio.is_null()) {
+      downloader_cat.info()
+        << "Connection lost while reading HTTP response.\n";
+      if (_response_type == RT_http_hangup) {
+        // This was our second hangup in a row.  Give up.
+        _state = S_failure;
+        
+      } else {
+        // Try again, once.
+        _response_type = RT_http_hangup;
+      }
+
+    } else if (ClockObject::get_global_clock()->get_real_time() -
+               _sent_request_time > get_http_timeout()) {
+      // Time to give up.
+      downloader_cat.info()
+        << "Timeout waiting for " << _url.get_server_and_port() << ".\n";
+      _state = S_failure;
+    }
     return true;
   }
+  _response_type = RT_http_complete;
+
+  // Ok, we've established an HTTP connection to the server.  Our
+  // extra send headers have done their job; clear them for next time.
+  clear_extra_headers();
 
   _server_response_has_no_body = 
     (get_status_code() / 100 == 1 ||
@@ -1490,6 +1535,7 @@ begin_request(HTTPEnum::Method method, const URLSpec &url,
 
   _first_byte = first_byte;
   _last_byte = last_byte;
+  _connect_count = 0;
 
   make_header();
   make_request_text();
@@ -1752,9 +1798,6 @@ parse_http_response(const string &line) {
     return false;
   }
 
-  // Okay, we're sane.
-  _response_type = RT_http;
-
   // Split out the first line into its three components.
   size_t p = 5;
   while (p < line.length() && !isspace(line[p])) {

panda/src/downloader/httpChannel.h

@@ -94,6 +94,9 @@ PUBLISHED:
   INLINE void set_blocking_connect(bool blocking_connect);
   INLINE bool get_blocking_connect() const;
 
+  INLINE void set_http_timeout(double timeout_seconds);
+  INLINE double get_http_timeout() const;
+
   INLINE void set_download_throttle(bool download_throttle);
   INLINE bool get_download_throttle() const;
 
@@ -204,6 +207,7 @@ private:
   PT(BioStreamPtr) _source;
   bool _persistent_connection;
   double _connect_timeout;
+  double _http_timeout;
   bool _blocking_connect;
   bool _download_throttle;
   double _max_bytes_per_second;
@@ -223,6 +227,7 @@ private:
   bool _server_response_has_no_body;
   size_t _first_byte;
   size_t _last_byte;
+  int _connect_count;
 
   enum DownloadDest {
     DD_none,
@@ -251,11 +256,13 @@ private:
   string _www_username;
   PT(HTTPAuthorization) _www_auth;
 
+  // What type of response do we get to our HTTP request?
   enum ResponseType {
     RT_none,
-    RT_hangup,
-    RT_non_http,
-    RT_http
+    RT_hangup,       // immediately lost connection 
+    RT_non_http,     // something that wasn't an expected HTTP response
+    RT_http_hangup,  // the start of an HTTP response, then a lost connection
+    RT_http_complete // a valid HTTP response completed
   };
   ResponseType _response_type;
   
@@ -295,6 +302,7 @@ private:
   State _state;
   State _done_state;
   double _started_connecting_time;
+  double _sent_request_time;
   bool _started_download;
   string _proxy_header;
   string _proxy_request_text;