From 1461e165010662e67c294f0968f4e85968d0bc7e Mon Sep 17 00:00:00 2001
From: Pavel Emelyanov <xemul@scylladb.com>
Date: Tue, 17 Oct 2023 13:29:38 +0300
Subject: [PATCH] http/client: Retry request over fresh connection in case old
 one failed

Currently if client::make_request() fails, the error is propagated back
to the caller whatever the error is. Sometimes the request fails because
of connection glitch, which can happen and in that case it's common to
try the same request again after a while in a hope that it was indeed
some teporary glitch.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
---
 include/seastar/http/client.hh |  7 ++++++-
 src/http/client.cc             | 23 +++++++++++++++++++++--
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/seastar/http/client.hh b/include/seastar/http/client.hh
index 21396a5e382..11cccf98535 100644
--- a/include/seastar/http/client.hh
+++ b/include/seastar/http/client.hh
@@ -168,6 +168,7 @@ public:
 class client {
 public:
     using reply_handler = noncopyable_function<future<>(const reply&, input_stream<char>&& body)>;
+    using retry_requests = bool_class<struct retry_requests_tag>;
 
 private:
     friend class http::internal::client_ref;
@@ -178,6 +179,7 @@ private:
     unsigned _nr_connections = 0;
     unsigned _max_connections;
     unsigned long _total_new_connections = 0;
+    const retry_requests _retry;
     condition_variable _wait_con;
     connections_list_t _pool;
 
@@ -232,7 +234,7 @@ public:
      * \param f -- the factory pointer
      *
      */
-    explicit client(std::unique_ptr<connection_factory> f, unsigned max_connections = default_max_connections);
+    explicit client(std::unique_ptr<connection_factory> f, unsigned max_connections = default_max_connections, retry_requests retry = retry_requests::no);
 
     /**
      * \brief Send the request and handle the response
@@ -246,6 +248,9 @@ public:
      * \param handle -- the response handler
      * \param expected -- the optional expected reply status code, default is std::nullopt
      *
+     * Note that the handle callback should be prepared to be called more than once, because
+     * client may restart the whole request processing in case server closes the connection
+     * in the middle of operation
      */
     future<> make_request(request req, reply_handler handle, std::optional<reply::status_type> expected = std::nullopt);
 
diff --git a/src/http/client.cc b/src/http/client.cc
index 01a93a242c9..53664a68017 100644
--- a/src/http/client.cc
+++ b/src/http/client.cc
@@ -236,9 +236,10 @@ client::client(socket_address addr, shared_ptr<tls::certificate_credentials> cre
 {
 }
 
-client::client(std::unique_ptr<connection_factory> f, unsigned max_connections)
+client::client(std::unique_ptr<connection_factory> f, unsigned max_connections, retry_requests retry)
         : _new_connections(std::move(f))
         , _max_connections(max_connections)
+        , _retry(retry)
 {
 }
 
@@ -330,9 +331,27 @@ auto client::with_new_connection(Fn&& fn) {
 
 future<> client::make_request(request req, reply_handler handle, std::optional<reply::status_type> expected) {
     return do_with(std::move(req), std::move(handle), [this, expected] (request& req, reply_handler& handle) mutable {
-        return with_connection([this, &req, &handle, expected] (connection& con) {
+        auto f = with_connection([this, &req, &handle, expected] (connection& con) {
             return do_make_request(con, req, handle, expected);
         });
+
+        if (_retry) {
+            f = f.handle_exception_type([this, &req, &handle, expected] (const std::system_error& ex) {
+                auto code = ex.code().value();
+                if ((code != EPIPE) && (code != ECONNABORTED)) {
+                    return make_exception_future<>(ex);
+                }
+
+                // The 'con' connection may not yet be freed, so the total connection
+                // count still account for it and with_new_connection() may temporarily
+                // break the limit. That's OK, the 'con' will be closed really soon
+                return with_new_connection([this, &req, &handle, expected] (connection& con) {
+                    return do_make_request(con, req, handle, expected);
+                });
+            });
+        }
+
+        return f;
     });
 }