In HTTP/3, write Priority on control stream before writing headers.
Currently the priority is in gQUIC fashion, where we only use the weight to build priority queue.
Priority might be sent on the control stream before handshake is confirmed. So this CL modifies QuicSendControlStream to send stream type and settings before any other data is sent.
gfe-relnote: v99 only, not in prod.
PiperOrigin-RevId: 256285943
Change-Id: Iaf72f9d6256a2692b284e3f6142755ff0d04d710
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index 17a3c82..4529430 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -25,14 +25,24 @@
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
}
- bool OnPriorityFrameStart(Http3FrameLengths /*frame_lengths*/) override {
- CloseConnectionOnWrongFrame("Priority");
- return false;
+ bool OnPriorityFrameStart(Http3FrameLengths frame_lengths) override {
+ if (stream_->session()->perspective() == Perspective::IS_CLIENT) {
+ stream_->session()->connection()->CloseConnection(
+ QUIC_HTTP_DECODER_ERROR, "Server must not send Priority frames.",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ return false;
+ }
+ return stream_->OnPriorityFrameStart(frame_lengths);
}
- bool OnPriorityFrame(const PriorityFrame& /*frame*/) override {
- CloseConnectionOnWrongFrame("Priority");
- return false;
+ bool OnPriorityFrame(const PriorityFrame& frame) override {
+ if (stream_->session()->perspective() == Perspective::IS_CLIENT) {
+ stream_->session()->connection()->CloseConnection(
+ QUIC_HTTP_DECODER_ERROR, "Server must not send Priority frames.",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ return false;
+ }
+ return stream_->OnPriorityFrame(frame);
}
bool OnCancelPushFrame(const CancelPushFrame& /*frame*/) override {
@@ -122,6 +132,7 @@
QuicReceiveControlStream::QuicReceiveControlStream(PendingStream* pending)
: QuicStream(pending, READ_UNIDIRECTIONAL, /*is_static=*/true),
+ current_priority_length_(0),
received_settings_length_(0),
http_decoder_visitor_(new HttpDecoderVisitor(this)),
sequencer_offset_(sequencer()->NumBytesConsumed()) {
@@ -190,4 +201,28 @@
return true;
}
+bool QuicReceiveControlStream::OnPriorityFrameStart(
+ Http3FrameLengths frame_lengths) {
+ DCHECK_EQ(Perspective::IS_SERVER, session()->perspective());
+ DCHECK_EQ(0u, current_priority_length_);
+ current_priority_length_ =
+ frame_lengths.header_length + frame_lengths.payload_length;
+ return true;
+}
+
+bool QuicReceiveControlStream::OnPriorityFrame(const PriorityFrame& priority) {
+ DCHECK_EQ(Perspective::IS_SERVER, session()->perspective());
+ QuicStream* stream =
+ session()->GetOrCreateStream(priority.prioritized_element_id);
+ // It's possible that the client sends a Priority frame for a request stream
+ // that the server is not permitted to open. In that case, simply drop the
+ // frame.
+ if (stream) {
+ stream->SetPriority(priority.weight);
+ }
+ sequencer()->MarkConsumed(current_priority_length_);
+ current_priority_length_ = 0;
+ return true;
+}
+
} // namespace quic
diff --git a/quic/core/http/quic_receive_control_stream.h b/quic/core/http/quic_receive_control_stream.h
index e4e9c20..3ea53cf 100644
--- a/quic/core/http/quic_receive_control_stream.h
+++ b/quic/core/http/quic_receive_control_stream.h
@@ -31,16 +31,21 @@
void SetUnblocked() { sequencer()->SetUnblocked(); }
- protected:
- // Called from HttpDecoderVisitor.
- bool OnSettingsFrameStart(Http3FrameLengths frame_lengths);
- bool OnSettingsFrame(const SettingsFrame& settings);
-
private:
class HttpDecoderVisitor;
+ // Called from HttpDecoderVisitor.
+ bool OnSettingsFrameStart(Http3FrameLengths frame_lengths);
+ bool OnSettingsFrame(const SettingsFrame& settings);
+ bool OnPriorityFrameStart(Http3FrameLengths frame_lengths);
+ // TODO(renjietang): Decode Priority in HTTP/3 style.
+ bool OnPriorityFrame(const PriorityFrame& priority);
+
HttpDecoder decoder_;
+ // Track the current priority frame length.
+ QuicByteCount current_priority_length_;
+
// Track the number of settings bytes received.
size_t received_settings_length_;
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index 9af2678..95fe8a2 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -45,6 +45,15 @@
return params;
}
+class TestStream : public QuicSpdyStream {
+ public:
+ TestStream(QuicStreamId id, QuicSpdySession* session)
+ : QuicSpdyStream(id, session, BIDIRECTIONAL) {}
+ ~TestStream() override = default;
+
+ void OnBodyAvailable() override {}
+};
+
class QuicReceiveControlStreamTest : public QuicTestWithParam<TestParams> {
public:
QuicReceiveControlStreamTest()
@@ -62,6 +71,10 @@
&session_);
receive_control_stream_ =
QuicMakeUnique<QuicReceiveControlStream>(pending.get());
+ stream_ = new TestStream(GetNthClientInitiatedBidirectionalStreamId(
+ GetParam().version.transport_version, 0),
+ &session_);
+ session_.ActivateStream(QuicWrapUnique(stream_));
}
Perspective perspective() const { return GetParam().perspective; }
@@ -73,12 +86,21 @@
return std::string(buffer.get(), header_length);
}
+ std::string PriorityFrame(const PriorityFrame& frame) {
+ HttpEncoder encoder;
+ std::unique_ptr<char[]> priority_buffer;
+ QuicByteCount priority_frame_length =
+ encoder.SerializePriorityFrame(frame, &priority_buffer);
+ return std::string(priority_buffer.get(), priority_frame_length);
+ }
+
MockQuicConnectionHelper helper_;
MockAlarmFactory alarm_factory_;
StrictMock<MockQuicConnection>* connection_;
StrictMock<MockQuicSpdySession> session_;
HttpDecoder decoder_;
std::unique_ptr<QuicReceiveControlStream> receive_control_stream_;
+ TestStream* stream_;
};
INSTANTIATE_TEST_SUITE_P(Tests,
@@ -154,6 +176,24 @@
receive_control_stream_->OnStreamFrame(frame);
}
+TEST_P(QuicReceiveControlStreamTest, ReceivePriorityFrame) {
+ if (perspective() == Perspective::IS_CLIENT) {
+ return;
+ }
+ struct PriorityFrame frame;
+ frame.prioritized_type = REQUEST_STREAM;
+ frame.dependency_type = ROOT_OF_TREE;
+ frame.prioritized_element_id = stream_->id();
+ frame.weight = 1;
+ std::string serialized_frame = PriorityFrame(frame);
+ QuicStreamFrame data(receive_control_stream_->id(), false, 0,
+ QuicStringPiece(serialized_frame));
+
+ EXPECT_EQ(3u, stream_->priority());
+ receive_control_stream_->OnStreamFrame(data);
+ EXPECT_EQ(1u, stream_->priority());
+}
+
TEST_P(QuicReceiveControlStreamTest, PushPromiseOnControlStreamShouldClose) {
PushPromiseFrame push_promise;
push_promise.push_id = 0x01;
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc
index f5f34f1..7943bdc 100644
--- a/quic/core/http/quic_send_control_stream.cc
+++ b/quic/core/http/quic_send_control_stream.cc
@@ -9,10 +9,13 @@
namespace quic {
-QuicSendControlStream::QuicSendControlStream(QuicStreamId id,
- QuicSpdySession* session)
+QuicSendControlStream::QuicSendControlStream(
+ QuicStreamId id,
+ QuicSpdySession* session,
+ uint64_t max_inbound_header_list_size)
: QuicStream(id, session, /*is_static = */ true, WRITE_UNIDIRECTIONAL),
- settings_sent_(false) {}
+ settings_sent_(false),
+ max_inbound_header_list_size_(max_inbound_header_list_size) {}
void QuicSendControlStream::OnStreamReset(const QuicRstStreamFrame& /*frame*/) {
// TODO(renjietang) Change the error code to H/3 specific
@@ -22,8 +25,10 @@
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
}
-void QuicSendControlStream::SendSettingsFrame(const SettingsFrame& settings) {
- DCHECK(!settings_sent_);
+void QuicSendControlStream::SendSettingsFrame() {
+ if (settings_sent_) {
+ return;
+ }
QuicConnection::ScopedPacketFlusher flusher(session()->connection());
// Send the stream type on so the peer knows about this stream.
@@ -33,6 +38,8 @@
WriteOrBufferData(QuicStringPiece(writer.data(), writer.length()), false,
nullptr);
+ SettingsFrame settings;
+ settings.values[kSettingsMaxHeaderListSize] = max_inbound_header_list_size_;
std::unique_ptr<char[]> buffer;
QuicByteCount frame_length =
encoder_.SerializeSettingsFrame(settings, &buffer);
@@ -43,4 +50,16 @@
settings_sent_ = true;
}
+void QuicSendControlStream::WritePriority(const PriorityFrame& priority) {
+ QuicConnection::ScopedPacketFlusher flusher(session()->connection());
+ if (!settings_sent_) {
+ SendSettingsFrame();
+ }
+ std::unique_ptr<char[]> buffer;
+ QuicByteCount frame_length =
+ encoder_.SerializePriorityFrame(priority, &buffer);
+ WriteOrBufferData(QuicStringPiece(buffer.get(), frame_length), false,
+ nullptr);
+}
+
} // namespace quic
diff --git a/quic/core/http/quic_send_control_stream.h b/quic/core/http/quic_send_control_stream.h
index aa5689b..44f5762 100644
--- a/quic/core/http/quic_send_control_stream.h
+++ b/quic/core/http/quic_send_control_stream.h
@@ -19,7 +19,9 @@
public:
// |session| can't be nullptr, and the ownership is not passed. The stream can
// only be accessed through the session.
- explicit QuicSendControlStream(QuicStreamId id, QuicSpdySession* session);
+ explicit QuicSendControlStream(QuicStreamId id,
+ QuicSpdySession* session,
+ uint64_t max_inbound_header_list_size);
QuicSendControlStream(const QuicSendControlStream&) = delete;
QuicSendControlStream& operator=(const QuicSendControlStream&) = delete;
~QuicSendControlStream() override = default;
@@ -28,9 +30,12 @@
// closed before connection.
void OnStreamReset(const QuicRstStreamFrame& frame) override;
- // Send |settings| on this stream.
- // Settings frame must be the first frame sent on this stream.
- void SendSettingsFrame(const SettingsFrame& settings);
+ // Consult the Spdy session to construct Settings frame and sends it on this
+ // stream. Settings frame must be the first frame sent on this stream.
+ void SendSettingsFrame();
+
+ // Send |Priority| on this stream. It must be sent after settings.
+ void WritePriority(const PriorityFrame& priority);
// The send control stream is write unidirectional, so this method should
// never be called.
@@ -40,6 +45,9 @@
HttpEncoder encoder_;
// Track if a settings frame is already sent.
bool settings_sent_;
+
+ // Max inbound header list size that will send as setting.
+ const uint64_t max_inbound_header_list_size_;
};
} // namespace quic
diff --git a/quic/core/http/quic_send_control_stream_test.cc b/quic/core/http/quic_send_control_stream_test.cc
index 83a4d6c..7c0ee12 100644
--- a/quic/core/http/quic_send_control_stream_test.cc
+++ b/quic/core/http/quic_send_control_stream_test.cc
@@ -56,7 +56,7 @@
session_.Initialize();
send_control_stream_ = QuicMakeUnique<QuicSendControlStream>(
QuicSpdySessionPeer::GetNextOutgoingUnidirectionalStreamId(&session_),
- &session_);
+ &session_, /* max_inbound_header_list_size = */ 100);
ON_CALL(session_, WritevData(_, _, _, _, _))
.WillByDefault(Invoke(MockQuicSession::ConsumeData));
}
@@ -75,22 +75,40 @@
QuicSendControlStreamTest,
::testing::ValuesIn(GetTestParams()));
-TEST_P(QuicSendControlStreamTest, WriteSettingsOnStartUp) {
+TEST_P(QuicSendControlStreamTest, WriteSettingsOnlyForOnce) {
if (GetParam().version.handshake_protocol == PROTOCOL_TLS1_3) {
// TODO(nharper, b/112643533): Figure out why this test fails when TLS is
// enabled and fix it.
return;
}
- SettingsFrame settings;
- settings.values[3] = 2;
- settings.values[6] = 5;
- std::unique_ptr<char[]> buffer;
- QuicByteCount frame_length =
- encoder_.SerializeSettingsFrame(settings, &buffer);
+ testing::InSequence s;
EXPECT_CALL(session_, WritevData(_, _, 1, _, _));
- EXPECT_CALL(session_, WritevData(_, _, frame_length, _, _));
- send_control_stream_->SendSettingsFrame(settings);
+ EXPECT_CALL(session_, WritevData(_, _, _, _, _));
+ send_control_stream_->SendSettingsFrame();
+
+ // No data should be written the sencond time SendSettingsFrame() is called.
+ send_control_stream_->SendSettingsFrame();
+}
+
+TEST_P(QuicSendControlStreamTest, WritePriorityBeforeSettings) {
+ if (GetParam().version.handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(nharper, b/112643533): Figure out why this test fails when TLS is
+ // enabled and fix it.
+ return;
+ }
+
+ testing::InSequence s;
+
+ // The first write will trigger the control stream to write stream type and a
+ // Settings frame before the Priority frame.
+ EXPECT_CALL(session_, WritevData(_, send_control_stream_->id(), _, _, _))
+ .Times(3);
+ PriorityFrame frame;
+ send_control_stream_->WritePriority(frame);
+
+ EXPECT_CALL(session_, WritevData(_, send_control_stream_->id(), _, _, _));
+ send_control_stream_->WritePriority(frame);
}
TEST_P(QuicSendControlStreamTest, ResetControlStream) {
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 3ef610e..0cd5f90 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -389,7 +389,8 @@
if (VersionHasStreamType(connection()->transport_version()) &&
eliminate_static_stream_map()) {
auto send_control = QuicMakeUnique<QuicSendControlStream>(
- GetNextOutgoingUnidirectionalStreamId(), this);
+ GetNextOutgoingUnidirectionalStreamId(), this,
+ max_inbound_header_list_size_);
send_control_stream_ = send_control.get();
RegisterStaticStreamNew(std::move(send_control),
/*stream_already_counted = */ false);
@@ -507,6 +508,14 @@
return frame.size();
}
+void QuicSpdySession::WriteH3Priority(const PriorityFrame& priority) {
+ DCHECK(VersionHasStreamType(connection()->transport_version()));
+ DCHECK(perspective() == Perspective::IS_CLIENT)
+ << "Server must not send priority";
+
+ send_control_stream_->WritePriority(priority);
+}
+
size_t QuicSpdySession::WritePushPromise(QuicStreamId original_stream_id,
QuicStreamId promised_stream_id,
SpdyHeaderBlock headers) {
@@ -529,9 +538,7 @@
void QuicSpdySession::SendMaxHeaderListSize(size_t value) {
if (VersionHasStreamType(connection()->transport_version())) {
- SettingsFrame settings;
- settings.values[kSettingsMaxHeaderListSize] = value;
- send_control_stream_->SendSettingsFrame(settings);
+ send_control_stream_->SendSettingsFrame();
return;
}
SpdySettingsIR settings_frame;
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index 1b5927a..a0ddac4 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -132,6 +132,9 @@
int weight,
bool exclusive);
+ // Writes a HTTP/3 PRIORITY frame to the peer.
+ void WriteH3Priority(const PriorityFrame& priority);
+
// Write |headers| for |promised_stream_id| on |original_stream_id| in a
// PUSH_PROMISE frame to peer.
// Return the size, in bytes, of the resulting PUSH_PROMISE frame.
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 9966f81..c625410 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -168,6 +168,7 @@
trailers_length_(0, 0),
trailers_decompressed_(false),
trailers_consumed_(false),
+ priority_sent_(false),
headers_bytes_to_be_marked_consumed_(0),
pretend_blocked_decoding_for_tests_(false),
http_decoder_visitor_(new HttpDecoderVisitor(this)),
@@ -204,6 +205,7 @@
trailers_length_(0, 0),
trailers_decompressed_(false),
trailers_consumed_(false),
+ priority_sent_(false),
headers_bytes_to_be_marked_consumed_(0),
pretend_blocked_decoding_for_tests_(false),
http_decoder_visitor_(new HttpDecoderVisitor(this)),
@@ -920,6 +922,13 @@
std::move(ack_listener));
}
+ if (session()->perspective() == Perspective::IS_CLIENT && !priority_sent_) {
+ PriorityFrame frame;
+ PopulatePriorityFrame(&frame);
+ spdy_session_->WriteH3Priority(frame);
+ priority_sent_ = true;
+ }
+
// Encode header list.
std::string encoded_headers =
spdy_session_->qpack_encoder()->EncodeHeaderList(id(), &header_block);
@@ -948,5 +957,12 @@
return encoded_headers.size();
}
+void QuicSpdyStream::PopulatePriorityFrame(PriorityFrame* frame) {
+ frame->weight = priority();
+ frame->dependency_type = ROOT_OF_TREE;
+ frame->prioritized_type = REQUEST_STREAM;
+ frame->prioritized_element_id = id();
+}
+
#undef ENDPOINT // undef for jumbo builds
} // namespace quic
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h
index 9a59ddc..0c2ba2d 100644
--- a/quic/core/http/quic_spdy_stream.h
+++ b/quic/core/http/quic_spdy_stream.h
@@ -238,6 +238,9 @@
ack_listener_ = std::move(ack_listener);
}
+ // Fills in |frame| with appropriate fields.
+ virtual void PopulatePriorityFrame(PriorityFrame* frame);
+
private:
friend class test::QuicSpdyStreamPeer;
friend class test::QuicStreamPeer;
@@ -287,6 +290,10 @@
bool trailers_decompressed_;
// True if the trailers have been consumed.
bool trailers_consumed_;
+
+ // True if the stream has already sent an priority frame.
+ bool priority_sent_;
+
// Number of bytes consumed while decoding HEADERS frames that cannot be
// marked consumed in QuicStreamSequencer until later.
QuicByteCount headers_bytes_to_be_marked_consumed_;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index b50c3bb..ff81ebf 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -166,9 +166,14 @@
}
void Initialize(bool stream_should_process_data) {
+ InitializeWithPerspective(stream_should_process_data,
+ Perspective::IS_SERVER);
+ }
+
+ void InitializeWithPerspective(bool stream_should_process_data,
+ Perspective perspective) {
connection_ = new StrictMock<MockQuicConnection>(
- &helper_, &alarm_factory_, Perspective::IS_SERVER,
- SupportedVersions(GetParam()));
+ &helper_, &alarm_factory_, perspective, SupportedVersions(GetParam()));
session_ = QuicMakeUnique<StrictMock<MockQuicSpdySession>>(connection_);
session_->Initialize();
ON_CALL(*session_, WritevData(_, _, _, _, _))
@@ -1163,6 +1168,42 @@
EXPECT_TRUE(stream_->fin_sent());
}
+TEST_P(QuicSpdyStreamTest, ClientWritesPriority) {
+ if (GetParam().handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(nharper, b/112643533): Figure out why this test fails when TLS is
+ // enabled and fix it.
+ return;
+ }
+
+ InitializeWithPerspective(kShouldProcessData, Perspective::IS_CLIENT);
+
+ if (VersionUsesQpack(GetParam().transport_version)) {
+ // In this case, TestStream::WriteHeadersImpl() does not prevent writes.
+ // Six writes include priority for headers, headers frame header, headers
+ // frame, priority of trailers, trailing headers frame header, and trailers.
+ EXPECT_CALL(*session_, WritevData(stream_, stream_->id(), _, _, _))
+ .Times(4);
+ auto send_control_stream =
+ QuicSpdySessionPeer::GetSendControlStream(session_.get());
+ // The control stream will write 3 times, including stream type, settings
+ // frame, priority for headers.
+ EXPECT_CALL(*session_, WritevData(send_control_stream,
+ send_control_stream->id(), _, _, _))
+ .Times(3);
+ }
+
+ // Write the initial headers, without a FIN.
+ EXPECT_CALL(*stream_, WriteHeadersMock(false));
+ stream_->WriteHeaders(SpdyHeaderBlock(), /*fin=*/false, nullptr);
+
+ // Writing trailers implicitly sends a FIN.
+ SpdyHeaderBlock trailers;
+ trailers["trailer key"] = "trailer value";
+ EXPECT_CALL(*stream_, WriteHeadersMock(true));
+ stream_->WriteTrailers(std::move(trailers), nullptr);
+ EXPECT_TRUE(stream_->fin_sent());
+}
+
// Test that when writing trailers, the trailers that are actually sent to the
// peer contain the final offset field indicating last byte of data.
TEST_P(QuicSpdyStreamTest, WritingTrailersFinalOffset) {
diff --git a/quic/test_tools/quic_spdy_session_peer.cc b/quic/test_tools/quic_spdy_session_peer.cc
index 6a49bb7..a28803c 100644
--- a/quic/test_tools/quic_spdy_session_peer.cc
+++ b/quic/test_tools/quic_spdy_session_peer.cc
@@ -87,5 +87,11 @@
return session->receive_control_stream_;
}
+// static
+QuicSendControlStream* QuicSpdySessionPeer::GetSendControlStream(
+ QuicSpdySession* session) {
+ return session->send_control_stream_;
+}
+
} // namespace test
} // namespace quic
diff --git a/quic/test_tools/quic_spdy_session_peer.h b/quic/test_tools/quic_spdy_session_peer.h
index 47ff021..c17735e 100644
--- a/quic/test_tools/quic_spdy_session_peer.h
+++ b/quic/test_tools/quic_spdy_session_peer.h
@@ -6,6 +6,7 @@
#define QUICHE_QUIC_TEST_TOOLS_QUIC_SPDY_SESSION_PEER_H_
#include "net/third_party/quiche/src/quic/core/http/quic_receive_control_stream.h"
+#include "net/third_party/quiche/src/quic/core/http/quic_send_control_stream.h"
#include "net/third_party/quiche/src/quic/core/quic_packets.h"
#include "net/third_party/quiche/src/quic/core/quic_write_blocked_list.h"
#include "net/third_party/quiche/src/spdy/core/spdy_framer.h"
@@ -49,6 +50,7 @@
QuicSpdySession* session);
static QuicReceiveControlStream* GetReceiveControlStream(
QuicSpdySession* session);
+ static QuicSendControlStream* GetSendControlStream(QuicSpdySession* session);
};
} // namespace test
