Http10 without keep-alive means close (#74)

* Make Http10 mean Connection: close.

Signed-off-by: John Plevyak <[email protected]>

source/common/http/codec_client.h

@@ -78,6 +78,11 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
    */
   uint64_t id() { return connection_->id(); }
 
+  /**
+   * @return the underlying codec protocol.
+   */
+  Protocol protocol() { return codec_->protocol(); }
+
   /**
    * @return the underlying connection error.
    */

source/common/http/http1/conn_pool.cc

@@ -203,7 +203,9 @@ 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 &&
+              !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) {
@@ -273,11 +275,15 @@ 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();
+    } else if (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
+                               Headers::get().ConnectionValues.KeepAlive)) {
+      saw_keep_alive_header_ = true;
+    }
   }
 
   StreamDecoderWrapper::decodeHeaders(std::move(headers), end_stream);

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_{};
   };
 

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,13 @@ 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 +533,8 @@ 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 +551,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());
@@ -578,6 +592,39 @@ 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<Http::MockStreamDecoder> outer_decoder;
+  ConnPoolCallbacks callbacks;
+  conn_pool_.expectClientCreate();
+  Http::ConnectionPool::Cancellable* handle = conn_pool_.newStream(outer_decoder, callbacks);
+
+  EXPECT_NE(nullptr, handle);
+
+  NiceMock<Http::MockStreamEncoder> 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 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"}});
+  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.
  */
@@ -605,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"}});
+  Http::HeaderMapPtr response_headers(
+      new TestHeaderMapImpl{{":status", "200"}, {"connection", "keep-alive"}});
   inner_decoder->decodeHeaders(std::move(response_headers), true);
   dispatcher_.clearDeferredDeleteList();
 
@@ -629,13 +677,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);