From 8f809b935affac8749f66f2426b9e4f1c2065904 Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Wed, 29 May 2019 17:14:15 -0700 Subject: [PATCH 1/6] Make Http10 mean Connection: close. Signed-off-by: John Plevyak --- source/common/http/codec_client.h | 5 +++++ source/common/http/http1/conn_pool.cc | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/source/common/http/codec_client.h b/source/common/http/codec_client.h index 0f4dff8bd8ab8..caae81485fccd 100644 --- a/source/common/http/codec_client.h +++ b/source/common/http/codec_client.h @@ -78,6 +78,11 @@ class CodecClient : Logger::Loggable, */ uint64_t id() { return connection_->id(); } + /** + * @return the underlying codec protocol. + */ + Protocol protocol() { return codec_->protocol(); } + /** * @return the underlying connection error. */ diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index e2f7a85656541..d5a21792311b0 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -203,7 +203,7 @@ void ConnPoolImpl::onResponseComplete(ActiveClient& client) { if (!client.stream_wrapper_->encode_complete_) { ENVOY_CONN_LOG(debug, "response before request complete", *client.codec_client_); onDownstreamReset(client); - } else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed()) { + } else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed() || client.codec_client_->protocol() == Protocol::Http10) { ENVOY_CONN_LOG(debug, "saw upstream connection: close", *client.codec_client_); onDownstreamReset(client); } else if (client.remaining_requests_ > 0 && --client.remaining_requests_ == 0) { From cf8d134406df1733312b55e673d4e32db83e2fd8 Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Wed, 29 May 2019 17:15:56 -0700 Subject: [PATCH 2/6] Make Http10 mean Connection: close. Signed-off-by: John Plevyak --- source/common/http/http1/conn_pool.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index d5a21792311b0..cb05ec06de0bf 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -203,7 +203,8 @@ void ConnPoolImpl::onResponseComplete(ActiveClient& client) { if (!client.stream_wrapper_->encode_complete_) { ENVOY_CONN_LOG(debug, "response before request complete", *client.codec_client_); onDownstreamReset(client); - } else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed() || client.codec_client_->protocol() == Protocol::Http10) { + } else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed() || + client.codec_client_->protocol() == Protocol::Http10) { ENVOY_CONN_LOG(debug, "saw upstream connection: close", *client.codec_client_); onDownstreamReset(client); } else if (client.remaining_requests_ > 0 && --client.remaining_requests_ == 0) { From f87a6f999cfce59c71d287450f5d66d975112b16 Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Thu, 30 May 2019 09:14:15 -0700 Subject: [PATCH 3/6] Address comments. Signed-off-by: John Plevyak --- source/common/http/http1/conn_pool.cc | 18 ++++++++++++------ source/common/http/http1/conn_pool.h | 1 + 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index cb05ec06de0bf..319250dfa20c7 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -204,7 +204,8 @@ void ConnPoolImpl::onResponseComplete(ActiveClient& client) { ENVOY_CONN_LOG(debug, "response before request complete", *client.codec_client_); onDownstreamReset(client); } else if (client.stream_wrapper_->saw_close_header_ || client.codec_client_->remoteClosed() || - client.codec_client_->protocol() == Protocol::Http10) { + (client.codec_client_->protocol() == Protocol::Http10 && + !client.stream_wrapper_->saw_keep_alive_header_)) { ENVOY_CONN_LOG(debug, "saw upstream connection: close", *client.codec_client_); onDownstreamReset(client); } else if (client.remaining_requests_ > 0 && --client.remaining_requests_ == 0) { @@ -274,11 +275,16 @@ ConnPoolImpl::StreamWrapper::~StreamWrapper() { void ConnPoolImpl::StreamWrapper::onEncodeComplete() { encode_complete_ = true; } void ConnPoolImpl::StreamWrapper::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { - if (headers->Connection() && - absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(), - Headers::get().ConnectionValues.Close)) { - saw_close_header_ = true; - parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc(); + if (headers->Connection()) { + if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(), + Headers::get().ConnectionValues.Close)) { + saw_close_header_ = true; + parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc(); + } + if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(), + Headers::get().ConnectionValues.KeepAlive)) { + saw_keep_alive_header_ = true; + } } StreamDecoderWrapper::decodeHeaders(std::move(headers), end_stream); diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index 9f28b0ac8c587..09c4bc780a579 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -74,6 +74,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase { ActiveClient& parent_; bool encode_complete_{}; bool saw_close_header_{}; + bool saw_keep_alive_header_{}; bool decode_complete_{}; }; From 07e00629ade1e72c6294382c17f1b16eec8fa15a Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Thu, 30 May 2019 12:26:11 -0700 Subject: [PATCH 4/6] Add test. Signed-off-by: John Plevyak --- test/common/http/http1/conn_pool_test.cc | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 27324c4a6dbf6..a9f779dd2eed4 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -578,6 +578,40 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseHeader) { EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); } +/** + * Test when upstream is HTTP/1.0 and does not send 'connection: keep-alive' + */ +TEST_F(Http1ConnPoolImplTest, NoConnectionKeepAlive) { + InSequence s; + + // Request 1 should kick off a new connection. + NiceMock outer_decoder; + ConnPoolCallbacks callbacks; + conn_pool_.expectClientCreate(); + Http::ConnectionPool::Cancellable* handle = conn_pool_.newStream(outer_decoder, callbacks); + + EXPECT_NE(nullptr, handle); + + NiceMock request_encoder; + Http::StreamDecoder* inner_decoder; + EXPECT_CALL(*conn_pool_.test_clients_[0].codec_, newStream(_)) + .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); + EXPECT_CALL(callbacks.pool_ready_, ready()); + + conn_pool_.test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); + + // Response with 'connection: close' which should cause the connection to go away. + EXPECT_CALL(conn_pool_, onClientDestroy()); + Http::HeaderMapPtr response_headers( + new TestHeaderMapImpl{{":protocol", "HTTP/1.0"}, {":status", "200"}}); + inner_decoder->decodeHeaders(std::move(response_headers), true); + dispatcher_.clearDeferredDeleteList(); + + EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); +} + + /** * Test when we reach max requests per connection. */ From f5e42581b3c97febf8ae3dfb4ac9374057908aaa Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Thu, 30 May 2019 13:06:04 -0700 Subject: [PATCH 5/6] Add tests. Signed-off-by: John Plevyak --- test/common/http/http1/conn_pool_test.cc | 32 ++++++++++++++++-------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index a9f779dd2eed4..72e04d9b91e7a 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -186,6 +186,18 @@ struct ActiveTestRequest { } } + void completeKeepAliveResponse(bool with_body) { + // Test additional metric writes also. + Http::HeaderMapPtr response_headers( + new TestHeaderMapImpl{{"connection", "keep-alive"}, {":status", "200"}, {"x-envoy-upstream-canary", "true"}}); + + inner_decoder_->decodeHeaders(std::move(response_headers), !with_body); + if (with_body) { + Buffer::OwnedImpl data; + inner_decoder_->decodeData(data, true); + } + } + void expectNewStream() { EXPECT_CALL(*parent_.conn_pool_.test_clients_[client_index_].codec_, newStream(_)) .WillOnce(DoAll(SaveArgAddress(&inner_decoder_), ReturnRef(request_encoder_))); @@ -275,7 +287,7 @@ TEST_F(Http1ConnPoolImplTest, MultipleRequestAndResponse) { // Request 1 should kick off a new connection. ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); r1.startRequest(); - r1.completeResponse(false); + r1.completeKeepAliveResponse(false); // Request 2 should not. ActiveTestRequest r2(*this, 0, ActiveTestRequest::Type::Immediate); @@ -470,12 +482,12 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { EXPECT_CALL(callbacks2.pool_ready_, ready()); callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}}); + Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_.expectAndRunUpstreamReady(); callbacks2.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - response_headers.reset(new TestHeaderMapImpl{{":status", "200"}}); + response_headers.reset(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); // Cause the connection to go away. @@ -520,7 +532,7 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { conn_pool_.expectEnableUpstreamReady(); callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}}); + Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); // Cause the connection to go away. @@ -537,7 +549,7 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { conn_pool_.test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); callbacks2.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - response_headers.reset(new TestHeaderMapImpl{{":status", "200"}}); + response_headers.reset(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_CALL(conn_pool_, onClientDestroy()); @@ -601,7 +613,7 @@ TEST_F(Http1ConnPoolImplTest, NoConnectionKeepAlive) { conn_pool_.test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - // Response with 'connection: close' which should cause the connection to go away. + // Response without 'connection: keep-alive' which should cause the connection to go away. EXPECT_CALL(conn_pool_, onClientDestroy()); Http::HeaderMapPtr response_headers( new TestHeaderMapImpl{{":protocol", "HTTP/1.0"}, {":status", "200"}}); @@ -639,7 +651,7 @@ TEST_F(Http1ConnPoolImplTest, MaxRequestsPerConnection) { // Response with 'connection: close' which should cause the connection to go away. EXPECT_CALL(conn_pool_, onClientDestroy()); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}}); + Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); dispatcher_.clearDeferredDeleteList(); @@ -663,13 +675,13 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { conn_pool_.expectEnableUpstreamReady(); r3.expectNewStream(); - r1.completeResponse(false); + r1.completeKeepAliveResponse(false); conn_pool_.expectAndRunUpstreamReady(); r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); - r2.completeResponse(false); - r3.completeResponse(false); + r2.completeKeepAliveResponse(false); + r3.completeKeepAliveResponse(false); // Disconnect both clients. EXPECT_CALL(conn_pool_, onClientDestroy()).Times(2); From 614126df326ef6cdfef276468de95bc9b421683d Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Thu, 30 May 2019 14:14:38 -0700 Subject: [PATCH 6/6] Address comments. Signed-off-by: John Plevyak --- source/common/http/http1/conn_pool.cc | 3 +-- test/common/http/http1/conn_pool_test.cc | 14 ++++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 319250dfa20c7..7d54ebc9adaab 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -280,8 +280,7 @@ void ConnPoolImpl::StreamWrapper::decodeHeaders(HeaderMapPtr&& headers, bool end Headers::get().ConnectionValues.Close)) { saw_close_header_ = true; parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc(); - } - if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(), + } else if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(), Headers::get().ConnectionValues.KeepAlive)) { saw_keep_alive_header_ = true; } diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 72e04d9b91e7a..2d8becae24e95 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -188,8 +188,8 @@ struct ActiveTestRequest { void completeKeepAliveResponse(bool with_body) { // Test additional metric writes also. - Http::HeaderMapPtr response_headers( - new TestHeaderMapImpl{{"connection", "keep-alive"}, {":status", "200"}, {"x-envoy-upstream-canary", "true"}}); + Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{ + {"connection", "keep-alive"}, {":status", "200"}, {"x-envoy-upstream-canary", "true"}}); inner_decoder_->decodeHeaders(std::move(response_headers), !with_body); if (with_body) { @@ -482,7 +482,8 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { EXPECT_CALL(callbacks2.pool_ready_, ready()); callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); + Http::HeaderMapPtr response_headers( + new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_.expectAndRunUpstreamReady(); @@ -532,7 +533,8 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { conn_pool_.expectEnableUpstreamReady(); callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); + Http::HeaderMapPtr response_headers( + new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); // Cause the connection to go away. @@ -623,7 +625,6 @@ TEST_F(Http1ConnPoolImplTest, NoConnectionKeepAlive) { EXPECT_EQ(0U, cluster_->stats_.upstream_cx_destroy_with_active_rq_.value()); } - /** * Test when we reach max requests per connection. */ @@ -651,7 +652,8 @@ TEST_F(Http1ConnPoolImplTest, MaxRequestsPerConnection) { // Response with 'connection: close' which should cause the connection to go away. EXPECT_CALL(conn_pool_, onClientDestroy()); - Http::HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); + Http::HeaderMapPtr response_headers( + new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}}); inner_decoder->decodeHeaders(std::move(response_headers), true); dispatcher_.clearDeferredDeleteList();