Make HTTP/1.0 responses without Connection: keep-alive mean close.

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,8 +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()) {
-    ENVOY_CONN_LOG(debug, "saw upstream connection: close", *client.codec_client_);
+  } else if (client.stream_wrapper_->close_connection_ || client.codec_client_->remoteClosed()) {
+    ENVOY_CONN_LOG(debug, "saw upstream close connection", *client.codec_client_);
     onDownstreamReset(client);
   } else if (client.remaining_requests_ > 0 && --client.remaining_requests_ == 0) {
     ENVOY_CONN_LOG(debug, "maximum requests per connection", *client.codec_client_);
@@ -273,17 +273,21 @@ 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 (!saw_close_header_ && headers->ProxyConnection() &&
-      absl::EqualsIgnoreCase(headers->ProxyConnection()->value().getStringView(),
-                             Headers::get().ConnectionValues.Close)) {
-    saw_close_header_ = true;
+  // If Connection: close OR
+  //    Http/1.0 and not Connection: keep-alive OR
+  //    Proxy-Connection: close
+  if ((headers->Connection() &&
+       (absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
+                               Headers::get().ConnectionValues.Close))) ||
+      (parent_.codec_client_->protocol() == Protocol::Http10 &&
+       (!headers->Connection() ||
+        !absl::EqualsIgnoreCase(headers->Connection()->value().getStringView(),
+                                Headers::get().ConnectionValues.KeepAlive))) ||
+      (headers->ProxyConnection() &&
+       (absl::EqualsIgnoreCase(headers->ProxyConnection()->value().getStringView(),
+                               Headers::get().ConnectionValues.Close)))) {
     parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc();
+    close_connection_ = true;
   }
 
   StreamDecoderWrapper::decodeHeaders(std::move(headers), end_stream);

source/common/http/http1/conn_pool.h

@@ -74,7 +74,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase {
 
     ActiveClient& parent_;
     bool encode_complete_{};
-    bool saw_close_header_{};
+    bool close_connection_{};
     bool decode_complete_{};
   };
 

test/common/http/http1/conn_pool_test.cc

@@ -74,7 +74,7 @@ class ConnPoolImplForTest : public ConnPoolImpl {
   MOCK_METHOD0(createCodecClient_, CodecClient*());
   MOCK_METHOD0(onClientDestroy, void());
 
-  void expectClientCreate() {
+  void expectClientCreate(Protocol protocol = Protocol::Http11) {
     test_clients_.emplace_back();
     TestCodecClient& test_client = test_clients_.back();
     test_client.connection_ = new NiceMock<Network::MockClientConnection>();
@@ -99,6 +99,7 @@ class ConnPoolImplForTest : public ConnPoolImpl {
         .WillOnce(Return(test_client.connection_));
     EXPECT_CALL(*this, createCodecClient_()).WillOnce(Return(test_client.codec_client_));
     EXPECT_CALL(*test_client.connect_timer_, enableTimer(_));
+    ON_CALL(*test_client.codec_, protocol()).WillByDefault(Return(protocol));
   }
 
   void expectEnableUpstreamReady() {
@@ -480,7 +481,9 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) {
 
   conn_pool_.expectAndRunUpstreamReady();
   callbacks2.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true);
-  response_headers.reset(new TestHeaderMapImpl{{":status", "200"}});
+  // N.B. clang_tidy insists that we use std::make_unique which can not infer std::initialize_list.
+  response_headers = std::make_unique<TestHeaderMapImpl>(
+      std::initializer_list<std::pair<std::string, std::string>>{{":status", "200"}});
   inner_decoder->decodeHeaders(std::move(response_headers), true);
 
   // Cause the connection to go away.
@@ -542,7 +545,9 @@ 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"}});
+  // N.B. clang_tidy insists that we use std::make_unique which can not infer std::initialize_list.
+  response_headers = std::make_unique<TestHeaderMapImpl>(
+      std::initializer_list<std::pair<std::string, std::string>>{{":status", "200"}});
   inner_decoder->decodeHeaders(std::move(response_headers), true);
 
   EXPECT_CALL(conn_pool_, onClientDestroy());
@@ -616,6 +621,39 @@ TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeader) {
   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, Http10NoConnectionKeepAlive) {
+  InSequence s;
+
+  // Request 1 should kick off a new connection.
+  NiceMock<Http::MockStreamDecoder> outer_decoder;
+  ConnPoolCallbacks callbacks;
+  conn_pool_.expectClientCreate(Protocol::Http10);
+  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.
  */