http2: fix stream flush timeout race with protocol error (envoyproxy#181

)

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Tony Allen <tony@allen.gg>

source/common/http/http2/codec_impl.cc

@@ -93,12 +93,7 @@ ConnectionImpl::StreamImpl::StreamImpl(ConnectionImpl& parent, uint32_t buffer_l
 ConnectionImpl::StreamImpl::~StreamImpl() { ASSERT(stream_idle_timer_ == nullptr); }
 
 void ConnectionImpl::StreamImpl::destroy() {
-  if (stream_idle_timer_ != nullptr) {
-    // To ease testing and the destructor assertion.
-    stream_idle_timer_->disableTimer();
-    stream_idle_timer_.reset();
-  }
-
+  disarmStreamIdleTimer();
   parent_.stats_.streams_active_.dec();
   parent_.stats_.pending_send_bytes_.sub(pending_send_data_.length());
 }
@@ -686,6 +681,15 @@ int ConnectionImpl::onFrameSend(const nghttp2_frame* frame) {
   case NGHTTP2_GOAWAY: {
     ENVOY_CONN_LOG(debug, "sent goaway code={}", connection_, frame->goaway.error_code);
     if (frame->goaway.error_code != NGHTTP2_NO_ERROR) {
+      // TODO(mattklein123): Returning this error code abandons standard nghttp2 frame accounting.
+      // As such, it is not reliable to call sendPendingFrames() again after this and we assume
+      // that the connection is going to get torn down immediately. One byproduct of this is that
+      // we need to cancel all pending flush stream timeouts since they can race with connection
+      // teardown. As part of the work to remove exceptions we should aim to clean up all of this
+      // error handling logic and only handle this type of case at the end of dispatch.
+      for (auto& stream : active_streams_) {
+        stream->disarmStreamIdleTimer();
+      }
       return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     break;

source/common/http/http2/codec_impl.h

@@ -158,6 +158,13 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
     // deferred delete lifetime issues that need sorting out if the destructor of the stream is
     // going to be able to refer to the parent connection.
     void destroy();
+    void disarmStreamIdleTimer() {
+      if (stream_idle_timer_ != nullptr) {
+        // To ease testing and the destructor assertion.
+        stream_idle_timer_->disableTimer();
+        stream_idle_timer_.reset();
+      }
+    }
 
     StreamImpl* base() { return this; }
     ssize_t onDataSourceRead(uint64_t length, uint32_t* data_flags);

test/common/http/http2/codec_impl_test.cc

@@ -1070,6 +1070,40 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeout) {
   EXPECT_EQ(1, server_stats_store_.counter("http2.tx_flush_timeout").value());
 }
 
+// Verify that when an incoming protocol error races with a stream flush timeout we correctly
+// disable the flush timeout and do not attempt to reset the stream.
+TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeoutAfterGoaway) {
+  initialize();
+
+  InSequence s;
+  MockStreamCallbacks client_stream_callbacks;
+  request_encoder_->getStream().addCallbacks(client_stream_callbacks);
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  request_encoder_->encodeHeaders(request_headers, true);
+
+  ON_CALL(client_connection_, write(_, _))
+      .WillByDefault(
+          Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); }));
+  TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  EXPECT_CALL(response_decoder_, decodeHeaders_(_, false));
+  response_encoder_->encodeHeaders(response_headers, false);
+  EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1));
+  auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_);
+  EXPECT_CALL(*flush_timer, enableTimer(std::chrono::milliseconds(30000), _));
+  Buffer::OwnedImpl body(std::string(1024 * 1024, 'a'));
+  response_encoder_->encodeData(body, true);
+
+  // Force a protocol error.
+  Buffer::OwnedImpl garbage_data("this should cause a protocol error");
+  EXPECT_CALL(client_callbacks_, onGoAway());
+  EXPECT_CALL(*flush_timer, disableTimer());
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0);
+  EXPECT_THROW(server_wrapper_.dispatch(garbage_data, *server_), CodecProtocolException);
+  EXPECT_EQ(0, server_stats_store_.counter("http2.tx_flush_timeout").value());
+}
+
 TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) {
   initialize();
   MockStreamCallbacks callbacks;