From f9897f8ccde5b85a81c221744b9f397d78451857 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 9 Jun 2016 23:17:41 +0900 Subject: [PATCH] nghttpx: Fix bugs and crash when affinity is enabled --- src/shrpx_api_downstream_connection.cc | 2 ++ src/shrpx_api_downstream_connection.h | 1 + src/shrpx_client_handler.cc | 21 +++++++++++++----- src/shrpx_downstream_connection.h | 2 ++ src/shrpx_http2_downstream_connection.cc | 2 ++ src/shrpx_http2_downstream_connection.h | 1 + src/shrpx_http_downstream_connection.cc | 28 +++++++++++++++++++----- src/shrpx_http_downstream_connection.h | 3 +-- src/shrpx_worker.h | 6 +++++ 9 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/shrpx_api_downstream_connection.cc b/src/shrpx_api_downstream_connection.cc index 5e13971d..55b3e01c 100644 --- a/src/shrpx_api_downstream_connection.cc +++ b/src/shrpx_api_downstream_connection.cc @@ -302,4 +302,6 @@ APIDownstreamConnection::get_downstream_addr_group() const { return nullptr; } +DownstreamAddr *APIDownstreamConnection::get_addr() const { return nullptr; } + } // namespace shrpx diff --git a/src/shrpx_api_downstream_connection.h b/src/shrpx_api_downstream_connection.h index fdf49526..f80341c7 100644 --- a/src/shrpx_api_downstream_connection.h +++ b/src/shrpx_api_downstream_connection.h @@ -55,6 +55,7 @@ public: virtual bool poolable() const; virtual DownstreamAddrGroup *get_downstream_addr_group() const; + virtual DownstreamAddr *get_addr() const; int send_reply(unsigned int http_status, int api_status); diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 3f98bc36..cfe95dfe 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -669,8 +669,18 @@ void ClientHandler::pool_downstream_connection( << " in group " << group; } - auto &dconn_pool = group->shared_addr->dconn_pool; - dconn_pool.add_downstream_connection(std::move(dconn)); + auto &shared_addr = group->shared_addr; + + if (shared_addr->affinity == AFFINITY_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) { @@ -715,7 +725,6 @@ Http2Session *ClientHandler::select_http2_session_with_affinity( if (addr->http2_extra_freelist.size()) { auto session = addr->http2_extra_freelist.head; - session->remove_from_freelist(); if (LOG_ENABLED(INFO)) { CLOG(INFO, this) << "Use Http2Session " << session @@ -727,8 +736,8 @@ Http2Session *ClientHandler::select_http2_session_with_affinity( CLOG(INFO, this) << "Maximum streams are reached for Http2Session(" << session << ")."; } - } else { - session->add_to_avail_freelist(); + + session->remove_from_freelist(); } return session; } @@ -740,7 +749,7 @@ Http2Session *ClientHandler::select_http2_session_with_affinity( CLOG(INFO, this) << "Create new Http2Session " << session; } - session->add_to_avail_freelist(); + session->add_to_extra_freelist(); return session; } diff --git a/src/shrpx_downstream_connection.h b/src/shrpx_downstream_connection.h index 7935099a..0372496d 100644 --- a/src/shrpx_downstream_connection.h +++ b/src/shrpx_downstream_connection.h @@ -35,6 +35,7 @@ class ClientHandler; class Upstream; class Downstream; struct DownstreamAddrGroup; +struct DownstreamAddr; class DownstreamConnection { public: @@ -61,6 +62,7 @@ public: virtual bool poolable() const = 0; virtual DownstreamAddrGroup *get_downstream_addr_group() const = 0; + virtual DownstreamAddr *get_addr() const = 0; void set_client_handler(ClientHandler *client_handler); ClientHandler *get_client_handler(); diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index bb7e60dc..c6213a11 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/src/shrpx_http2_downstream_connection.cc @@ -543,4 +543,6 @@ Http2DownstreamConnection::get_downstream_addr_group() const { return http2session_->get_downstream_addr_group(); } +DownstreamAddr *Http2DownstreamConnection::get_addr() const { return nullptr; } + } // namespace shrpx diff --git a/src/shrpx_http2_downstream_connection.h b/src/shrpx_http2_downstream_connection.h index 6f72a884..f8195560 100644 --- a/src/shrpx_http2_downstream_connection.h +++ b/src/shrpx_http2_downstream_connection.h @@ -65,6 +65,7 @@ public: virtual bool poolable() const { return false; } virtual DownstreamAddrGroup *get_downstream_addr_group() const; + virtual DownstreamAddr *get_addr() const; int send(); diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index 77770a09..495147d3 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -562,6 +562,24 @@ int HttpDownstreamConnection::end_upload_data() { return 0; } +namespace { +void remove_from_pool(HttpDownstreamConnection *dconn) { + auto group = dconn->get_downstream_addr_group(); + auto &shared_addr = group->shared_addr; + + if (shared_addr->affinity == AFFINITY_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); +} +} // namespace + namespace { void idle_readcb(struct ev_loop *loop, ev_io *w, int revents) { auto conn = static_cast(w->data); @@ -569,9 +587,8 @@ void idle_readcb(struct ev_loop *loop, ev_io *w, int revents) { if (LOG_ENABLED(INFO)) { DCLOG(INFO, dconn) << "Idle connection EOF"; } - auto &dconn_pool = - dconn->get_downstream_addr_group()->shared_addr->dconn_pool; - dconn_pool.remove_downstream_connection(dconn); + + remove_from_pool(dconn); // dconn was deleted } } // namespace @@ -583,9 +600,8 @@ void idle_timeoutcb(struct ev_loop *loop, ev_timer *w, int revents) { if (LOG_ENABLED(INFO)) { DCLOG(INFO, dconn) << "Idle connection timeout"; } - auto &dconn_pool = - dconn->get_downstream_addr_group()->shared_addr->dconn_pool; - dconn_pool.remove_downstream_connection(dconn); + + remove_from_pool(dconn); // dconn was deleted } } // namespace diff --git a/src/shrpx_http_downstream_connection.h b/src/shrpx_http_downstream_connection.h index 48241e2d..8fad5353 100644 --- a/src/shrpx_http_downstream_connection.h +++ b/src/shrpx_http_downstream_connection.h @@ -65,6 +65,7 @@ public: virtual bool poolable() const; virtual DownstreamAddrGroup *get_downstream_addr_group() const; + virtual DownstreamAddr *get_addr() const; int read_clear(); int write_clear(); @@ -80,8 +81,6 @@ public: int noop(); - DownstreamAddr *get_addr() const; - private: Connection conn_; std::function do_read_, do_write_, diff --git a/src/shrpx_worker.h b/src/shrpx_worker.h index 26aabe38..2a0a2965 100644 --- a/src/shrpx_worker.h +++ b/src/shrpx_worker.h @@ -96,6 +96,9 @@ struct DownstreamAddr { // Http2Session object created for this address. This list chains // all Http2Session objects that is not in group scope // http2_avail_freelist, and is not reached in maximum concurrency. + // + // If session affinity is enabled, http2_avail_freelist is not used, + // and this list is solely used. DList http2_extra_freelist; // true if Http2Session for this address is in group scope // SharedDownstreamAddr.http2_avail_freelist @@ -129,6 +132,9 @@ struct SharedDownstreamAddr { // coalesce as much stream as possible in one Http2Session to fully // utilize TCP connection. // + // If session affinity is enabled, this list is not used. Per + // address http2_extra_freelist is used instead. + // // TODO Verify that this approach performs better in performance // wise. DList http2_avail_freelist;