From e9c9838cdc86d4e141435a2a8620c0d955cdf818 Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Fri, 11 Jan 2019 23:26:08 +0900
Subject: [PATCH] nghttpx: Pool h1 backend connection per address

Pool HTTP/1.1 backend connection per address and reuse it only when
the next round robin index refers to this address.  Previously if
there is a pooled connection, there is no round robin selection.
---
 src/shrpx_client_handler.cc             | 69 +++++++++++++------------
 src/shrpx_http_downstream_connection.cc | 10 ----
 src/shrpx_worker.cc                     | 12 +----
 src/shrpx_worker.h                      |  1 -
 4 files changed, 38 insertions(+), 54 deletions(-)

diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc
index cdb20146..71e522a0 100644
--- a/src/shrpx_client_handler.cc
+++ b/src/shrpx_client_handler.cc
@@ -658,30 +658,11 @@ void ClientHandler::pool_downstream_connection(
                      << " in group " << group;
   }
 
-  auto &shared_addr = group->shared_addr;
-
-  if (shared_addr->affinity.type == SessionAffinity::NONE) {
-    auto &dconn_pool = group->shared_addr->dconn_pool;
-    dconn_pool.add_downstream_connection(std::move(dconn));
-
-    return;
-  }
-
   auto addr = dconn->get_addr();
   auto &dconn_pool = addr->dconn_pool;
   dconn_pool->add_downstream_connection(std::move(dconn));
 }
 
-void ClientHandler::remove_downstream_connection(DownstreamConnection *dconn) {
-  if (LOG_ENABLED(INFO)) {
-    CLOG(INFO, this) << "Removing downstream connection DCONN:" << dconn
-                     << " from pool";
-  }
-  auto &dconn_pool =
-      dconn->get_downstream_addr_group()->shared_addr->dconn_pool;
-  dconn_pool.remove_downstream_connection(dconn);
-}
-
 namespace {
 // Computes 32bits hash for session affinity for IP address |ip|.
 uint32_t compute_affinity_from_ip(const StringRef &ip) {
@@ -1138,26 +1119,48 @@ ClientHandler::get_downstream_connection(int &err, Downstream *downstream,
     return std::move(dconn);
   }
 
-  auto &dconn_pool = shared_addr->dconn_pool;
+  auto end = shared_addr->next;
+  for (;;) {
+    auto addr = &shared_addr->addrs[shared_addr->next];
 
-  // pool connection must be HTTP/1.1 connection
-  auto dconn = dconn_pool.pop_downstream_connection();
+    if (addr->proto != Proto::HTTP1) {
+      if (++shared_addr->next >= shared_addr->addrs.size()) {
+        shared_addr->next = 0;
+      }
 
-  if (dconn) {
-    if (LOG_ENABLED(INFO)) {
-      CLOG(INFO, this) << "Reuse downstream connection DCONN:" << dconn.get()
-                       << " from pool";
-    }
-  } else {
-    if (LOG_ENABLED(INFO)) {
-      CLOG(INFO, this) << "Downstream connection pool is empty."
-                       << " Create new one";
+      assert(end != shared_addr->next);
+
+      continue;
     }
 
-    dconn = std::make_unique<HttpDownstreamConnection>(group, 0, conn_.loop,
-                                                       worker_);
+    // pool connection must be HTTP/1.1 connection
+    auto dconn = addr->dconn_pool->pop_downstream_connection();
+    if (dconn) {
+      if (++shared_addr->next >= shared_addr->addrs.size()) {
+        shared_addr->next = 0;
+      }
+
+      if (LOG_ENABLED(INFO)) {
+        CLOG(INFO, this) << "Reuse downstream connection DCONN:" << dconn.get()
+                         << " from pool";
+      }
+
+      dconn->set_client_handler(this);
+
+      return dconn;
+    }
+
+    break;
   }
 
+  if (LOG_ENABLED(INFO)) {
+    CLOG(INFO, this) << "Downstream connection pool is empty."
+                     << " Create new one";
+  }
+
+  auto dconn =
+      std::make_unique<HttpDownstreamConnection>(group, 0, conn_.loop, worker_);
+
   dconn->set_client_handler(this);
 
   return dconn;
diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc
index 6b4c28eb..1fea9b14 100644
--- a/src/shrpx_http_downstream_connection.cc
+++ b/src/shrpx_http_downstream_connection.cc
@@ -807,16 +807,6 @@ int HttpDownstreamConnection::end_upload_data() {
 
 namespace {
 void remove_from_pool(HttpDownstreamConnection *dconn) {
-  auto &group = dconn->get_downstream_addr_group();
-  auto &shared_addr = group->shared_addr;
-
-  if (shared_addr->affinity.type == SessionAffinity::NONE) {
-    auto &dconn_pool =
-        dconn->get_downstream_addr_group()->shared_addr->dconn_pool;
-    dconn_pool.remove_downstream_connection(dconn);
-    return;
-  }
-
   auto addr = dconn->get_addr();
   auto &dconn_pool = addr->dconn_pool;
   dconn_pool->remove_downstream_connection(dconn);
diff --git a/src/shrpx_worker.cc b/src/shrpx_worker.cc
index 8ce9869c..83b6fef2 100644
--- a/src/shrpx_worker.cc
+++ b/src/shrpx_worker.cc
@@ -164,12 +164,6 @@ void Worker::replace_downstream_config(
     g->retired = true;
 
     auto &shared_addr = g->shared_addr;
-
-    if (shared_addr->affinity.type == SessionAffinity::NONE) {
-      shared_addr->dconn_pool.remove_all();
-      continue;
-    }
-
     for (auto &addr : shared_addr->addrs) {
       addr.dconn_pool->remove_all();
     }
@@ -307,10 +301,8 @@ void Worker::replace_downstream_config(
       std::shuffle(std::begin(shared_addr->addrs), std::end(shared_addr->addrs),
                    randgen_);
 
-      if (shared_addr->affinity.type != SessionAffinity::NONE) {
-        for (auto &addr : shared_addr->addrs) {
-          addr.dconn_pool = std::make_unique<DownstreamConnectionPool>();
-        }
+      for (auto &addr : shared_addr->addrs) {
+        addr.dconn_pool = std::make_unique<DownstreamConnectionPool>();
       }
 
       dst->shared_addr = shared_addr;
diff --git a/src/shrpx_worker.h b/src/shrpx_worker.h
index 3705b6fe..14e2e2de 100644
--- a/src/shrpx_worker.h
+++ b/src/shrpx_worker.h
@@ -164,7 +164,6 @@ struct SharedDownstreamAddr {
   // TODO Verify that this approach performs better in performance
   // wise.
   DList<Http2Session> http2_avail_freelist;
-  DownstreamConnectionPool dconn_pool;
   // Configuration for session affinity
   AffinityConfig affinity;
   // Next http/1.1 downstream address index in addrs.