snag final stream info before destruction

docs/root/intro/version_history.rst

@@ -16,6 +16,7 @@ Bugfixes:
 - iOS: fix CocoaPods releases (:issue:`#2215 <2215>`)
 - fix bug where writing prevented the read loop from running (:issue:`#2221 <2221>`)
 - Android: update Kotlin standard libraries to 1.6.21 (:issue:`#2256 <2256>`)
+- fix bug where finalStreamIntel was not consistently set on cancel (:issue:`#2285 <2285>`)
 - iOS: fix termination crash in ProvisionalDispatcher (:issue:`#2059 <2059>`)
 
 Features:

library/common/http/client.cc

@@ -269,9 +269,6 @@ void Client::DirectStreamCallbacks::onCancel() {
 
   ENVOY_LOG(debug, "[S{}] dispatching to platform cancel stream", direct_stream_.stream_handle_);
   http_client_.stats().stream_cancel_.inc();
-  // Attempt to latch the latest stream info. This will be a no-op if the stream
-  // is already complete.
-  direct_stream_.saveFinalStreamIntel();
   bridge_callbacks_.on_cancel(streamIntel(), finalStreamIntel(), bridge_callbacks_.context);
 }
 
@@ -310,7 +307,6 @@ void Client::DirectStream::saveFinalStreamIntel() {
   if (!request_decoder_ || !parent_.getStream(stream_handle_, ALLOW_ONLY_FOR_OPEN_STREAMS)) {
     return;
   }
-
   StreamInfo::setFinalStreamIntel(request_decoder_->streamInfo(), parent_.dispatcher_.timeSource(),
                                   envoy_final_stream_intel_);
 }
@@ -517,6 +513,9 @@ void Client::cancelStream(envoy_stream_t stream) {
   Client::DirectStreamSharedPtr direct_stream =
       getStream(stream, GetStreamFilters::ALLOW_FOR_ALL_STREAMS);
   if (direct_stream) {
+    // Attempt to latch the latest stream info. This will be a no-op if the stream
+    // is already complete.
+    direct_stream->saveFinalStreamIntel();
     bool stream_was_open =
         getStream(stream, GetStreamFilters::ALLOW_ONLY_FOR_OPEN_STREAMS) != nullptr;
     ScopeTrackerScopeState scope(direct_stream.get(), scopeTracker());

test/common/integration/client_integration_test.cc

@@ -137,8 +137,9 @@ class ClientIntegrationTest : public BaseIntegrationTest,
       cc_->terminal_callback->setReady();
       return nullptr;
     };
-    bridge_callbacks_.on_cancel = [](envoy_stream_intel, envoy_final_stream_intel,
+    bridge_callbacks_.on_cancel = [](envoy_stream_intel, envoy_final_stream_intel final_intel,
                                      void* context) -> void* {
+      EXPECT_NE(-1, final_intel.stream_start_ms);
       callbacks_called* cc_ = static_cast<callbacks_called*>(context);
       cc_->on_cancel_calls++;
       cc_->terminal_callback->setReady();
@@ -353,6 +354,58 @@ TEST_P(ClientIntegrationTest, BasicCancel) {
   ASSERT_EQ(cc_.on_cancel_calls, 1);
 }
 
+TEST_P(ClientIntegrationTest, CancelWithPartialStream) {
+  autonomous_upstream_ = false;
+  initialize();
+
+  bridge_callbacks_.on_headers = [](envoy_headers c_headers, bool, envoy_stream_intel,
+                                    void* context) -> void* {
+    Http::ResponseHeaderMapPtr response_headers = toResponseHeaders(c_headers);
+    callbacks_called* cc_ = static_cast<callbacks_called*>(context);
+    cc_->on_headers_calls++;
+    cc_->status = response_headers->Status()->value().getStringView();
+    // Lie and say the request is complete, so the test has something to wait
+    // on.
+    cc_->terminal_callback->setReady();
+    return nullptr;
+  };
+
+  envoy_headers c_headers = Http::Utility::toBridgeHeaders(default_request_headers_);
+
+  // Create a stream with explicit flow control.
+  dispatcher_->post([&]() -> void {
+    http_client_->startStream(stream_, bridge_callbacks_, true);
+    http_client_->sendHeaders(stream_, c_headers, true);
+  });
+
+  Envoy::FakeRawConnectionPtr upstream_connection;
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection));
+
+  std::string upstream_request;
+  EXPECT_TRUE(upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("GET /"),
+                                               &upstream_request));
+
+  // Send a complete response with body.
+  auto response = "HTTP/1.1 200 OK\r\nContent-Length: 3\r\n\r\nasd";
+  ASSERT_TRUE(upstream_connection->write(response));
+  // For this test only, the terminal callback is called when headers arrive.
+  terminal_callback_.waitReady();
+  ASSERT_EQ(cc_.on_headers_calls, 1);
+  ASSERT_EQ(cc_.status, "200");
+  ASSERT_EQ(cc_.on_data_calls, 0);
+  ASSERT_EQ(cc_.on_complete_calls, 0);
+  // Due to explicit flow control, the upstream stream is complete, but the
+  // callbacks will not be called for data and completion. Cancel the stream
+  // and make sure the cancel is received.
+  dispatcher_->post([&]() -> void { http_client_->cancelStream(stream_); });
+  terminal_callback_.waitReady();
+  ASSERT_EQ(cc_.on_headers_calls, 1);
+  ASSERT_EQ(cc_.status, "200");
+  ASSERT_EQ(cc_.on_data_calls, 0);
+  ASSERT_EQ(cc_.on_complete_calls, 0);
+  ASSERT_EQ(cc_.on_cancel_calls, 1);
+}
+
 // TODO(junr03): test with envoy local reply with local stream not closed, which causes a reset
 // fired from the Http:ConnectionManager rather than the Http::Client. This cannot be done in
 // unit tests because the Http::ConnectionManager is mocked using a mock response encoder.