Fix issue of unexpected response timeouts when an HTTP client with lo…

…ng time keep-alive. see https://github.com/swoole/benchmark/blob/master/co_http_client_keepalive.php
swoole · Nov 6, 2024 · 3d8abe6 · 3d8abe6
1 parent 050293e
commit 3d8abe6
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 22 deletions.
diff --git a/ext-src/swoole_http_client_coro.cc b/ext-src/swoole_http_client_coro.cc
@@ -93,7 +93,8 @@ class Client {
 #ifdef SW_USE_OPENSSL
     uint8_t ssl;
 #endif
-    double connect_timeout = network::Socket::default_connect_timeout;
+    double connect_timeout = 0;
+    double response_timeout = 0;
     bool defer = false;
     bool lowercase_header = true;
     bool use_default_port;
@@ -706,10 +707,12 @@ void Client::apply_setting(zval *zset, const bool check_all) {
         zval *ztmp;
         HashTable *vht = Z_ARRVAL_P(zset);
 
-        if (php_swoole_array_get_value(vht, "connect_timeout", ztmp) ||
-            php_swoole_array_get_value(vht, "timeout", ztmp) /* backward compatibility */) {
+        if (php_swoole_array_get_value(vht, "connect_timeout", ztmp)) {
             connect_timeout = zval_get_double(ztmp);
         }
+        if (php_swoole_array_get_value(vht, "timeout", ztmp)) {
+            response_timeout = zval_get_double(ztmp);
+        }
         if (php_swoole_array_get_value(vht, "max_retries", ztmp)) {
             max_retries = (uint8_t) SW_MIN(zval_get_long(ztmp), UINT8_MAX);
         }
@@ -863,11 +866,11 @@ bool Client::connect() {
     accept_websocket_compression = false;
 #endif
 
-    // socket->set_buffer_allocator(&SWOOLE_G(zend_string_allocator));
-    // connect
-    socket->set_timeout(connect_timeout, Socket::TIMEOUT_CONNECT);
+    double _timeout = connect_timeout == 0 ? network::Socket::default_connect_timeout : connect_timeout;
+    socket->set_timeout(_timeout, Socket::TIMEOUT_CONNECT);
     socket->set_resolve_context(&resolve_context_);
     socket->set_dtor([this](Socket *_socket) { socket_dtor(); });
+    // socket->set_buffer_allocator(&SWOOLE_G(zend_string_allocator));
 
     if (!socket->connect(host, port)) {
         set_error(socket->errCode, socket->errMsg, ESTATUS_CONNECT_FAILED);
@@ -1425,9 +1428,9 @@ bool Client::recv_response(double timeout) {
     parser.data = this;
 
     if (timeout == 0) {
-        timeout = socket->get_timeout(Socket::TIMEOUT_READ);
+        timeout = response_timeout == 0 ? network::Socket::default_read_timeout : response_timeout;
     }
-    Socket::timeout_controller tc(socket, timeout, Socket::TIMEOUT_READ);
+    Socket::TimeoutController tc(socket, timeout, Socket::TIMEOUT_READ);
     bool success = false;
     while (true) {
         if (sw_unlikely(tc.has_timedout(Socket::TIMEOUT_READ))) {

diff --git a/include/swoole_coroutine_socket.h b/include/swoole_coroutine_socket.h
@@ -509,8 +509,8 @@ class Socket {
 
     class TimerController {
       public:
-        TimerController(TimerNode **timer_pp, double timeout, Socket *sock, TimerCallback callback)
-            : timer_pp(timer_pp), timeout(timeout), socket_(sock), callback(callback) {}
+        TimerController(TimerNode **_timer_pp, double _timeout, Socket *_socket, TimerCallback _callback)
+            : timer_pp(_timer_pp), timeout(_timeout), socket_(_socket), callback(std::move(_callback)) {}
         bool start() {
             if (timeout != 0 && !*timer_pp) {
                 enabled = true;
@@ -542,16 +542,16 @@ class Socket {
   public:
     class TimeoutSetter {
       public:
-        TimeoutSetter(Socket *socket, double timeout, const enum TimeoutType type)
-            : socket_(socket), timeout(timeout), type(type) {
-            if (timeout == 0) {
+        TimeoutSetter(Socket *socket, double _timeout, const enum TimeoutType _type)
+            : socket_(socket), timeout(_timeout), type(_type) {
+            if (_timeout == 0) {
                 return;
             }
             for (uint8_t i = 0; i < SW_ARRAY_SIZE(timeout_type_list); i++) {
-                if (type & timeout_type_list[i]) {
+                if (_type & timeout_type_list[i]) {
                     original_timeout[i] = socket->get_timeout(timeout_type_list[i]);
-                    if (timeout != original_timeout[i]) {
-                        socket->set_timeout(timeout, timeout_type_list[i]);
+                    if (_timeout != original_timeout[i]) {
+                        socket->set_timeout(_timeout, timeout_type_list[i]);
                     }
                 }
             }
@@ -576,12 +576,13 @@ class Socket {
         double original_timeout[sizeof(timeout_type_list)] = {};
     };
 
-    class timeout_controller : public TimeoutSetter {
+    class TimeoutController : public TimeoutSetter {
       public:
-        timeout_controller(Socket *socket, double timeout, const enum TimeoutType type)
-            : TimeoutSetter(socket, timeout, type) {}
-        bool has_timedout(const enum TimeoutType type) {
-            SW_ASSERT_1BYTE(type);
+        TimeoutController(Socket *_socket, double _timeout, const enum TimeoutType _type)
+            : TimeoutSetter(_socket, _timeout, _type) {}
+
+        bool has_timedout(const enum TimeoutType _type) {
+            SW_ASSERT_1BYTE(_type);
             if (timeout > 0) {
                 if (sw_unlikely(startup_time == 0)) {
                     startup_time = microtime();
@@ -591,7 +592,7 @@ class Socket {
                         socket_->set_err(ETIMEDOUT);
                         return true;
                     }
-                    socket_->set_timeout(timeout - used_time, type);
+                    socket_->set_timeout(timeout - used_time, _type);
                 }
             }
             return false;