Support multiple interim responses in SimpleClient. Currently, SimpleClient and descendants support 100 Continue responses (an interim response) by reading in 100 Continue to response_headers(). Then once 100 Continue is populated and verified, the client calls ClearPreliminaryResponse() so that response_headers() can be reused for the final response. This CL updates SimpleClient and descendants to support multiple interim responses by introducing interim_headers(), an ordered sequence of BalsaHeaders for interim responses. This CL removes ClearPreliminaryResponse() and adds WaitForInterimResponse(), which in the future may be called multiple times. SimpleHttpClient also calls BalsaFrame::set_use_interim_headers_callback(true), which gives some additional testing confidence in this new code path. This CL simplifies some 100-specific logic in the test clients. However, the client continues to treat 101 as a final response (even in SimpleHttp2Client, for consistency with SimpleHttpClient) due to the issue in cl/528583898. One part of this CL involves updates to the QUIC (test) client ecosystem. I decided to replace the interim/preliminary headers functionality in QuicTestClient, as updating interim headers on querying (rather than on receipt) can interact strangely with stream resets and timeouts. This CL instead plumbs a callback to update interim headers with the flow SimpleQuicClient --> GfeMockableQuicClient --> QuicSimpleClientSession --> QuicSimpleClientStream, which can immediately update interim headers on receipt. A comparison: $ blaze test //gfe/gfe2/e2e:expect_100_continue_test_ietf_quic Before: "PASSED in 307.2s" (http://sponge2/2c1eb63c-2bcc-4399-a329-98c638f562ba) After: "PASSED in 11.0s" (http://sponge2/6c178f44-0aa3-4684-a4b0-e52fdbe61ae2) I also verified with ++haoyuewang@ that the QuicSpdyClientStream changes do not have a production impact on Hyperloop. Huge inspiration from and thanks to ++bnc@'s foundational cl/379795244, which has made this CL possible! PiperOrigin-RevId: 529847861
diff --git a/quiche/quic/core/http/quic_spdy_client_stream.cc b/quiche/quic/core/http/quic_spdy_client_stream.cc index 4eded5e..2f6eb8d 100644 --- a/quiche/quic/core/http/quic_spdy_client_stream.cc +++ b/quiche/quic/core/http/quic_spdy_client_stream.cc
@@ -28,8 +28,7 @@ response_code_(0), header_bytes_read_(0), header_bytes_written_(0), - session_(session), - has_preliminary_headers_(false) {} + session_(session) {} QuicSpdyClientStream::QuicSpdyClientStream(PendingStream* pending, QuicSpdyClientSession* session) @@ -38,8 +37,7 @@ response_code_(0), header_bytes_read_(0), header_bytes_written_(0), - session_(session), - has_preliminary_headers_(false) {} + session_(session) {} QuicSpdyClientStream::~QuicSpdyClientStream() = default; @@ -74,13 +72,7 @@ << response_headers_[":status"].as_string() << " on stream " << id(); set_headers_decompressed(false); - if (response_code_ == 100 && !has_preliminary_headers_) { - // This is 100 Continue, save it to enable "Expect: 100-continue". - has_preliminary_headers_ = true; - preliminary_headers_ = std::move(response_headers_); - } else { - response_headers_.clear(); - } + preliminary_headers_.push_back(std::move(response_headers_)); } return true;
diff --git a/quiche/quic/core/http/quic_spdy_client_stream.h b/quiche/quic/core/http/quic_spdy_client_stream.h index 9a952aa..02956c6 100644 --- a/quiche/quic/core/http/quic_spdy_client_stream.h +++ b/quiche/quic/core/http/quic_spdy_client_stream.h
@@ -6,6 +6,7 @@ #define QUICHE_QUIC_CORE_HTTP_QUIC_SPDY_CLIENT_STREAM_H_ #include <cstddef> +#include <list> #include <string> #include "absl/strings/string_view.h" @@ -56,7 +57,7 @@ // Returns whatever headers have been received for this stream. const spdy::Http2HeaderBlock& response_headers() { return response_headers_; } - const spdy::Http2HeaderBlock& preliminary_headers() { + const std::list<spdy::Http2HeaderBlock>& preliminary_headers() { return preliminary_headers_; } @@ -96,11 +97,9 @@ QuicSpdyClientSession* session_; - // These preliminary headers are used for the 100 Continue headers - // that may arrive before the response headers when the request has - // Expect: 100-continue. - bool has_preliminary_headers_; - spdy::Http2HeaderBlock preliminary_headers_; + // These preliminary headers are used for interim response headers that may + // arrive before the final response headers. + std::list<spdy::Http2HeaderBlock> preliminary_headers_; }; } // namespace quic
diff --git a/quiche/quic/core/http/quic_spdy_client_stream_test.cc b/quiche/quic/core/http/quic_spdy_client_stream_test.cc index b249a98..2735b01 100644 --- a/quiche/quic/core/http/quic_spdy_client_stream_test.cc +++ b/quiche/quic/core/http/quic_spdy_client_stream_test.cc
@@ -166,7 +166,9 @@ auto headers = AsHeaderList(headers_); stream_->OnStreamHeaderList(false, headers.uncompressed_header_bytes(), headers); - EXPECT_EQ("100", stream_->preliminary_headers().find(":status")->second); + ASSERT_EQ(stream_->preliminary_headers().size(), 1); + EXPECT_EQ("100", + stream_->preliminary_headers().front().find(":status")->second); EXPECT_EQ(0u, stream_->response_headers().size()); EXPECT_EQ(100, stream_->response_code()); EXPECT_EQ("", stream_->data()); @@ -187,7 +189,9 @@ EXPECT_EQ(200, stream_->response_code()); EXPECT_EQ(body_, stream_->data()); // Make sure the 100 response is still available. - EXPECT_EQ("100", stream_->preliminary_headers().find(":status")->second); + ASSERT_EQ(stream_->preliminary_headers().size(), 1); + EXPECT_EQ("100", + stream_->preliminary_headers().front().find(":status")->second); } TEST_P(QuicSpdyClientStreamTest, TestUnknownInformationalBeforeSuccessful) { @@ -196,6 +200,9 @@ auto headers = AsHeaderList(headers_); stream_->OnStreamHeaderList(false, headers.uncompressed_header_bytes(), headers); + ASSERT_EQ(stream_->preliminary_headers().size(), 1); + EXPECT_EQ("199", + stream_->preliminary_headers().front().find(":status")->second); EXPECT_EQ(0u, stream_->response_headers().size()); EXPECT_EQ(199, stream_->response_code()); EXPECT_EQ("", stream_->data()); @@ -215,6 +222,63 @@ EXPECT_EQ("200", stream_->response_headers().find(":status")->second); EXPECT_EQ(200, stream_->response_code()); EXPECT_EQ(body_, stream_->data()); + // Make sure the 199 response is still available. + ASSERT_EQ(stream_->preliminary_headers().size(), 1); + EXPECT_EQ("199", + stream_->preliminary_headers().front().find(":status")->second); +} + +TEST_P(QuicSpdyClientStreamTest, TestMultipleInformationalBeforeSuccessful) { + // First send 100 Continue. + headers_[":status"] = "100"; + auto headers = AsHeaderList(headers_); + stream_->OnStreamHeaderList(false, headers.uncompressed_header_bytes(), + headers); + ASSERT_EQ(stream_->preliminary_headers().size(), 1); + EXPECT_EQ("100", + stream_->preliminary_headers().front().find(":status")->second); + EXPECT_EQ(0u, stream_->response_headers().size()); + EXPECT_EQ(100, stream_->response_code()); + EXPECT_EQ("", stream_->data()); + + // Then send 199, an unknown Informational (1XX). + headers_[":status"] = "199"; + headers = AsHeaderList(headers_); + stream_->OnStreamHeaderList(false, headers.uncompressed_header_bytes(), + headers); + ASSERT_EQ(stream_->preliminary_headers().size(), 2); + EXPECT_EQ("100", + stream_->preliminary_headers().front().find(":status")->second); + EXPECT_EQ("199", + stream_->preliminary_headers().back().find(":status")->second); + EXPECT_EQ(0u, stream_->response_headers().size()); + EXPECT_EQ(199, stream_->response_code()); + EXPECT_EQ("", stream_->data()); + + // Then send 200 OK. + headers_[":status"] = "200"; + headers = AsHeaderList(headers_); + stream_->OnStreamHeaderList(false, headers.uncompressed_header_bytes(), + headers); + quiche::QuicheBuffer header = HttpEncoder::SerializeDataFrameHeader( + body_.length(), quiche::SimpleBufferAllocator::Get()); + std::string data = VersionUsesHttp3(connection_->transport_version()) + ? absl::StrCat(header.AsStringView(), body_) + : body_; + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), /*fin=*/false, /*offset=*/0, data)); + + // Make sure the 200 response got parsed correctly. + EXPECT_EQ("200", stream_->response_headers().find(":status")->second); + EXPECT_EQ(200, stream_->response_code()); + EXPECT_EQ(body_, stream_->data()); + + // Make sure the informational responses are still available. + ASSERT_EQ(stream_->preliminary_headers().size(), 2); + EXPECT_EQ("100", + stream_->preliminary_headers().front().find(":status")->second); + EXPECT_EQ("199", + stream_->preliminary_headers().back().find(":status")->second); } TEST_P(QuicSpdyClientStreamTest, TestReceiving101) {
diff --git a/quiche/quic/test_tools/quic_test_client.cc b/quiche/quic/test_tools/quic_test_client.cc index 1f17227..5835800 100644 --- a/quiche/quic/test_tools/quic_test_client.cc +++ b/quiche/quic/test_tools/quic_test_client.cc
@@ -616,7 +616,6 @@ response_ = ""; response_complete_ = false; response_headers_complete_ = false; - preliminary_headers_.clear(); response_headers_.clear(); response_trailers_.clear(); bytes_read_ = 0; @@ -668,18 +667,6 @@ return &response_headers_; } -const spdy::Http2HeaderBlock* QuicTestClient::preliminary_headers() const { - for (std::pair<QuicStreamId, QuicSpdyClientStream*> stream : open_streams_) { - size_t bytes_read = - stream.second->stream_bytes_read() + stream.second->header_bytes_read(); - if (bytes_read > 0) { - preliminary_headers_ = stream.second->preliminary_headers().Clone(); - break; - } - } - return &preliminary_headers_; -} - const spdy::Http2HeaderBlock& QuicTestClient::response_trailers() const { return response_trailers_; } @@ -737,7 +724,6 @@ client_stream->stream_error(), connected(), client_stream->headers_decompressed(), client_stream->response_headers(), - client_stream->preliminary_headers(), (buffer_body() ? std::string(client_stream->data()) : ""), client_stream->received_trailers(), // Use NumBytesConsumed to avoid counting retransmitted stream frames. @@ -844,7 +830,6 @@ response_complete(other.response_complete), response_headers_complete(other.response_headers_complete), response_headers(other.response_headers.Clone()), - preliminary_headers(other.preliminary_headers.Clone()), response(other.response), response_trailers(other.response_trailers.Clone()), bytes_read(other.bytes_read), @@ -855,7 +840,6 @@ QuicRstStreamErrorCode stream_error, bool response_complete, bool response_headers_complete, const spdy::Http2HeaderBlock& response_headers, - const spdy::Http2HeaderBlock& preliminary_headers, const std::string& response, const spdy::Http2HeaderBlock& response_trailers, uint64_t bytes_read, uint64_t bytes_written, int64_t response_body_size) @@ -863,7 +847,6 @@ response_complete(response_complete), response_headers_complete(response_headers_complete), response_headers(response_headers.Clone()), - preliminary_headers(preliminary_headers.Clone()), response(response), response_trailers(response_trailers.Clone()), bytes_read(bytes_read), @@ -896,7 +879,6 @@ response_ = state.response; response_complete_ = state.response_complete; response_headers_complete_ = state.response_headers_complete; - preliminary_headers_ = state.preliminary_headers.Clone(); response_headers_ = state.response_headers.Clone(); response_trailers_ = state.response_trailers.Clone(); bytes_read_ = state.bytes_read;
diff --git a/quiche/quic/test_tools/quic_test_client.h b/quiche/quic/test_tools/quic_test_client.h index 9f2a869..4b95046 100644 --- a/quiche/quic/test_tools/quic_test_client.h +++ b/quiche/quic/test_tools/quic_test_client.h
@@ -176,7 +176,6 @@ // have received a partial response. bool response_headers_complete() const; const spdy::Http2HeaderBlock* response_headers() const; - const spdy::Http2HeaderBlock* preliminary_headers() const; int64_t response_size() const; size_t bytes_read() const; size_t bytes_written() const; @@ -358,7 +357,6 @@ PerStreamState(QuicRstStreamErrorCode stream_error, bool response_complete, bool response_headers_complete, const spdy::Http2HeaderBlock& response_headers, - const spdy::Http2HeaderBlock& preliminary_headers, const std::string& response, const spdy::Http2HeaderBlock& response_trailers, uint64_t bytes_read, uint64_t bytes_written, @@ -369,7 +367,6 @@ bool response_complete; bool response_headers_complete; spdy::Http2HeaderBlock response_headers; - spdy::Http2HeaderBlock preliminary_headers; std::string response; spdy::Http2HeaderBlock response_trailers; uint64_t bytes_read; @@ -402,7 +399,6 @@ bool response_complete_; bool response_headers_complete_; - mutable spdy::Http2HeaderBlock preliminary_headers_; mutable spdy::Http2HeaderBlock response_headers_; // Parsed response trailers (if present), copied from the stream in OnClose.
diff --git a/quiche/quic/tools/quic_simple_client_session.cc b/quiche/quic/tools/quic_simple_client_session.cc index eaaa921..0bb1515 100644 --- a/quiche/quic/tools/quic_simple_client_session.cc +++ b/quiche/quic/tools/quic_simple_client_session.cc
@@ -36,9 +36,11 @@ std::unique_ptr<QuicSpdyClientStream> QuicSimpleClientSession::CreateClientStream() { - return std::make_unique<QuicSimpleClientStream>( + auto stream = std::make_unique<QuicSimpleClientStream>( GetNextOutgoingBidirectionalStreamId(), this, BIDIRECTIONAL, drop_response_body_); + stream->set_on_interim_headers(on_interim_headers_); + return stream; } bool QuicSimpleClientSession::ShouldNegotiateWebTransport() {
diff --git a/quiche/quic/tools/quic_simple_client_session.h b/quiche/quic/tools/quic_simple_client_session.h index b172afc..2a0db5d 100644 --- a/quiche/quic/tools/quic_simple_client_session.h +++ b/quiche/quic/tools/quic_simple_client_session.h
@@ -5,9 +5,13 @@ #ifndef QUICHE_QUIC_TOOLS_QUIC_SIMPLE_CLIENT_SESSION_H_ #define QUICHE_QUIC_TOOLS_QUIC_SIMPLE_CLIENT_SESSION_H_ +#include <functional> +#include <utility> + #include "quiche/quic/core/http/quic_spdy_client_session.h" #include "quiche/quic/tools/quic_client_base.h" #include "quiche/quic/tools/quic_simple_client_stream.h" +#include "quiche/spdy/core/http2_header_block.h" namespace quic { @@ -42,7 +46,13 @@ std::unique_ptr<QuicPathValidationContext> context) override; bool drop_response_body() const { return drop_response_body_; } + void set_on_interim_headers( + std::function<void(const spdy::Http2HeaderBlock&)> on_interim_headers) { + on_interim_headers_ = std::move(on_interim_headers); + } + private: + std::function<void(const spdy::Http2HeaderBlock&)> on_interim_headers_; QuicClientBase::NetworkHelper* network_helper_; const bool drop_response_body_; const bool enable_web_transport_;
diff --git a/quiche/quic/tools/quic_simple_client_stream.cc b/quiche/quic/tools/quic_simple_client_stream.cc index 14de9f0..76dd39f 100644 --- a/quiche/quic/tools/quic_simple_client_stream.cc +++ b/quiche/quic/tools/quic_simple_client_stream.cc
@@ -4,6 +4,8 @@ #include "quiche/quic/tools/quic_simple_client_stream.h" +#include "quiche/common/platform/api/quiche_logging.h" + namespace quic { void QuicSimpleClientStream::OnBodyAvailable() { @@ -26,4 +28,20 @@ } } +bool QuicSimpleClientStream::ParseAndValidateStatusCode() { + const size_t num_previous_interim_headers = preliminary_headers().size(); + if (!QuicSpdyClientStream::ParseAndValidateStatusCode()) { + return false; + } + // The base ParseAndValidateStatusCode() may have added a preliminary header. + if (preliminary_headers().size() > num_previous_interim_headers) { + QUICHE_DCHECK_EQ(preliminary_headers().size(), + num_previous_interim_headers + 1); + if (on_interim_headers_) { + on_interim_headers_(preliminary_headers().back()); + } + } + return true; +} + } // namespace quic
diff --git a/quiche/quic/tools/quic_simple_client_stream.h b/quiche/quic/tools/quic_simple_client_stream.h index 976ceaf..fd7ede4 100644 --- a/quiche/quic/tools/quic_simple_client_stream.h +++ b/quiche/quic/tools/quic_simple_client_stream.h
@@ -5,6 +5,9 @@ #ifndef QUICHE_QUIC_TOOLS_QUIC_SIMPLE_CLIENT_STREAM_H_ #define QUICHE_QUIC_TOOLS_QUIC_SIMPLE_CLIENT_STREAM_H_ +#include <functional> +#include <utility> + #include "quiche/quic/core/http/quic_spdy_client_stream.h" namespace quic { @@ -17,7 +20,16 @@ drop_response_body_(drop_response_body) {} void OnBodyAvailable() override; + void set_on_interim_headers( + std::function<void(const spdy::Http2HeaderBlock&)> on_interim_headers) { + on_interim_headers_ = std::move(on_interim_headers); + } + + protected: + bool ParseAndValidateStatusCode() override; + private: + std::function<void(const spdy::Http2HeaderBlock&)> on_interim_headers_; const bool drop_response_body_; };
diff --git a/quiche/quic/tools/quic_spdy_client_base.cc b/quiche/quic/tools/quic_spdy_client_base.cc index b46e780..248e379 100644 --- a/quiche/quic/tools/quic_spdy_client_base.cc +++ b/quiche/quic/tools/quic_spdy_client_base.cc
@@ -7,6 +7,7 @@ #include <utility> #include "absl/strings/numbers.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quiche/quic/core/crypto/quic_random.h" #include "quiche/quic/core/http/spdy_utils.h" @@ -86,8 +87,10 @@ QUIC_LOG(ERROR) << "Invalid :status response header: " << status->second; } latest_response_headers_ = response_headers.DebugString(); - preliminary_response_headers_ = - client_stream->preliminary_headers().DebugString(); + for (const Http2HeaderBlock& headers : + client_stream->preliminary_headers()) { + absl::StrAppend(&preliminary_response_headers_, headers.DebugString()); + } latest_response_header_block_ = response_headers.Clone(); latest_response_body_ = std::string(client_stream->data()); latest_response_trailers_ =