diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 626bd0e2ac..5f8733b39d 100644 --- a/docs/root/intro/version_history.rst +++ b/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: diff --git a/library/common/http/client.cc b/library/common/http/client.cc index a16738cfd0..24cdc56952 100644 --- a/library/common/http/client.cc +++ b/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()); diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 090d7ff8b6..cb89f2322c 100644 --- a/test/common/integration/client_integration_test.cc +++ b/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(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(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.