Update goaway frame field name and type. not protected.
Motivation for type change: remote endpoints might send GOAWAY frames with ID
larger than what could be represented on QuicStreamId (uint32_t). It is
important that a value like 2^32+8 is not treated as 8 but as the largest
possible QuicStreamId. The ideal place to handle this is
QuicSpdySession::OnHttp3GoAway(), therefore this CL plumbs 64 bits there from
HttpDecoder::FinishParsing(). For the time being this only means moving the
explicit cast, so there is no behavior change.
There is no need to change SendHttp3GoAway() to handle 64 bit values, because
QuicStreamId is only 32 bits internally, so 2^32-4 is sufficient to send when
doing graceful shutdown (which is not yet implemented).
Motivation for name change: https://github.com/quicwg/base-drafts/pull/3129
introduces GOAWAY frames sent from client to server, in which case the payload
carries a push ID, not a stream ID.
PiperOrigin-RevId: 322393259
Change-Id: I472ec669259d675705790ee12c9404ca3d54195c
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index d6c97ee..3e06703 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -408,20 +408,14 @@
case static_cast<uint64_t>(HttpFrameType::GOAWAY): {
QuicDataReader reader(buffer_.data(), current_frame_length_);
GoAwayFrame frame;
- static_assert(!std::is_same<decltype(frame.stream_id), uint64_t>::value,
- "Please remove local |stream_id| variable and pass "
- "&frame.stream_id directly to ReadVarInt62() when changing "
- "QuicStreamId from uint32_t to uint64_t.");
- uint64_t stream_id;
- if (!reader.ReadVarInt62(&stream_id)) {
- RaiseError(QUIC_HTTP_FRAME_ERROR, "Unable to read GOAWAY stream_id.");
+ if (!reader.ReadVarInt62(&frame.id)) {
+ RaiseError(QUIC_HTTP_FRAME_ERROR, "Unable to read GOAWAY ID.");
return false;
}
if (!reader.IsDoneReading()) {
RaiseError(QUIC_HTTP_FRAME_ERROR, "Superfluous data in GOAWAY frame.");
return false;
}
- frame.stream_id = static_cast<QuicStreamId>(stream_id);
continue_processing = visitor_->OnGoAwayFrame(frame);
break;
}
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 86a1b69..01f0a5f 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -597,7 +597,7 @@
std::string input = quiche::QuicheTextUtils::HexDecode(
"07" // type (GOAWAY)
"01" // length
- "01"); // StreamId
+ "01"); // ID
// Visitor pauses processing.
EXPECT_CALL(visitor_, OnGoAwayFrame(GoAwayFrame({1})))
@@ -851,7 +851,7 @@
{"\x07" // type (GOAWAY)
"\x01" // length
"\x40", // first byte of two-byte varint stream id
- "Unable to read GOAWAY stream_id."},
+ "Unable to read GOAWAY ID."},
{"\x07" // type (GOAWAY)
"\x04" // length
"\x05" // valid stream id
@@ -928,7 +928,7 @@
EXPECT_CALL(visitor_, OnError(&decoder_));
EXPECT_EQ(input.size(), ProcessInput(input));
EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR));
- EXPECT_EQ("Unable to read GOAWAY stream_id.", decoder_.error_detail());
+ EXPECT_EQ("Unable to read GOAWAY ID.", decoder_.error_detail());
}
TEST_F(HttpDecoderTest, EmptyMaxPushIdFrame) {
@@ -944,7 +944,7 @@
TEST_F(HttpDecoderTest, LargeStreamIdInGoAway) {
GoAwayFrame frame;
- frame.stream_id = 1 << 30;
+ frame.id = 1ull << 60;
std::unique_ptr<char[]> buffer;
uint64_t length = HttpEncoder::SerializeGoAwayFrame(frame, &buffer);
EXPECT_CALL(visitor_, OnGoAwayFrame(frame));
diff --git a/quic/core/http/http_encoder.cc b/quic/core/http/http_encoder.cc
index a51abbe..7707705 100644
--- a/quic/core/http/http_encoder.cc
+++ b/quic/core/http/http_encoder.cc
@@ -161,8 +161,7 @@
QuicByteCount HttpEncoder::SerializeGoAwayFrame(
const GoAwayFrame& goaway,
std::unique_ptr<char[]>* output) {
- QuicByteCount payload_length =
- QuicDataWriter::GetVarInt62Len(goaway.stream_id);
+ QuicByteCount payload_length = QuicDataWriter::GetVarInt62Len(goaway.id);
QuicByteCount total_length =
GetTotalLength(payload_length, HttpFrameType::GOAWAY);
@@ -170,7 +169,7 @@
QuicDataWriter writer(total_length, output->get());
if (WriteFrameHeader(payload_length, HttpFrameType::GOAWAY, &writer) &&
- writer.WriteVarInt62(goaway.stream_id)) {
+ writer.WriteVarInt62(goaway.id)) {
return total_length;
}
QUIC_DLOG(ERROR)
diff --git a/quic/core/http/http_encoder_test.cc b/quic/core/http/http_encoder_test.cc
index aad20e5..f6c0444 100644
--- a/quic/core/http/http_encoder_test.cc
+++ b/quic/core/http/http_encoder_test.cc
@@ -102,12 +102,12 @@
TEST(HttpEncoderTest, SerializeGoAwayFrame) {
GoAwayFrame goaway;
- goaway.stream_id = 0x1;
+ goaway.id = 0x1;
char output[] = {// type (GOAWAY)
0x07,
// length
0x1,
- // StreamId
+ // ID
0x01};
std::unique_ptr<char[]> buffer;
uint64_t length = HttpEncoder::SerializeGoAwayFrame(goaway, &buffer);
diff --git a/quic/core/http/http_frames.h b/quic/core/http/http_frames.h
index 242f72b..159b250 100644
--- a/quic/core/http/http_frames.h
+++ b/quic/core/http/http_frames.h
@@ -107,14 +107,15 @@
// 7.2.6. GOAWAY
//
-// The GOAWAY frame (type=0x7) is used to initiate graceful shutdown of
-// a connection by a server.
+// The GOAWAY frame (type=0x7) is used to initiate shutdown of a connection by
+// either endpoint.
struct QUIC_EXPORT_PRIVATE GoAwayFrame {
- QuicStreamId stream_id;
+ // When sent from server to client, |id| is a stream ID that should refer to
+ // a client-initiated bidirectional stream.
+ // When sent from client to server, |id| is a push ID.
+ uint64_t id;
- bool operator==(const GoAwayFrame& rhs) const {
- return stream_id == rhs.stream_id;
- }
+ bool operator==(const GoAwayFrame& rhs) const { return id == rhs.id; }
};
// 7.2.7. MAX_PUSH_ID
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc
index 27de70d..e0e51a7 100644
--- a/quic/core/http/quic_receive_control_stream.cc
+++ b/quic/core/http/quic_receive_control_stream.cc
@@ -110,7 +110,7 @@
return false;
}
- spdy_session()->OnHttp3GoAway(frame.stream_id);
+ spdy_session()->OnHttp3GoAway(frame.id);
return true;
}
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc
index b7517b5..91e09f2 100644
--- a/quic/core/http/quic_receive_control_stream_test.cc
+++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -282,7 +282,7 @@
settings_frame));
offset += settings_frame.length();
- GoAwayFrame goaway{/* stream_id = */ 0};
+ GoAwayFrame goaway{/* id = */ 0};
std::unique_ptr<char[]> buffer;
QuicByteCount header_length =
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc
index 59574a1..7be8ee4 100644
--- a/quic/core/http/quic_send_control_stream.cc
+++ b/quic/core/http/quic_send_control_stream.cc
@@ -121,20 +121,19 @@
/*fin = */ false, nullptr);
}
-void QuicSendControlStream::SendGoAway(QuicStreamId stream_id) {
+void QuicSendControlStream::SendGoAway(QuicStreamId id) {
QuicConnection::ScopedPacketFlusher flusher(session()->connection());
MaybeSendSettingsFrame();
GoAwayFrame frame;
- // If the peer hasn't created any stream yet. Use stream id 0 to indicate no
+ // If the peer has not created any stream yet, use stream ID 0 to indicate no
// request is accepted.
- if (stream_id ==
- QuicUtils::GetInvalidStreamId(session()->transport_version())) {
- stream_id = 0;
+ if (id == QuicUtils::GetInvalidStreamId(session()->transport_version())) {
+ id = 0;
}
- frame.stream_id = stream_id;
+ frame.id = id;
if (spdy_session_->debug_visitor()) {
- spdy_session_->debug_visitor()->OnGoAwayFrameSent(stream_id);
+ spdy_session_->debug_visitor()->OnGoAwayFrameSent(id);
}
std::unique_ptr<char[]> buffer;
diff --git a/quic/core/http/quic_send_control_stream.h b/quic/core/http/quic_send_control_stream.h
index c14254f..6240310 100644
--- a/quic/core/http/quic_send_control_stream.h
+++ b/quic/core/http/quic_send_control_stream.h
@@ -47,7 +47,7 @@
// Send a GOAWAY frame on this stream, and a SETTINGS frame beforehand if one
// has not been already sent.
- void SendGoAway(QuicStreamId stream_id);
+ void SendGoAway(QuicStreamId id);
// The send control stream is write unidirectional, so this method should
// never be called.
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 850858e..b0bf7e9 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -655,8 +655,14 @@
send_control_stream_->WritePriorityUpdate(priority_update);
}
-void QuicSpdySession::OnHttp3GoAway(QuicStreamId stream_id) {
+void QuicSpdySession::OnHttp3GoAway(uint64_t id) {
DCHECK_EQ(perspective(), Perspective::IS_CLIENT);
+
+ // QuicStreamId is uint32_t. Casting to this narrower type is well-defined
+ // and preserves the lower 32 bits. Both IsBidirectionalStreamId() and
+ // IsIncomingStream() give correct results, because their return value is
+ // determined by the least significant two bits.
+ QuicStreamId stream_id = static_cast<QuicStreamId>(id);
if (!QuicUtils::IsBidirectionalStreamId(stream_id, version()) ||
IsIncomingStream(stream_id)) {
CloseConnectionWithDetails(
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index e437d98..9342da2 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -216,9 +216,9 @@
// Writes an HTTP/3 PRIORITY_UPDATE frame to the peer.
void WriteHttp3PriorityUpdate(const PriorityUpdateFrame& priority_update);
- // Process received HTTP/3 GOAWAY frame. This method should only be called on
- // the client side.
- virtual void OnHttp3GoAway(QuicStreamId stream_id);
+ // Process received HTTP/3 GOAWAY frame. When sent from server to client,
+ // |id| is a stream ID. When sent from client to server, |id| is a push ID.
+ virtual void OnHttp3GoAway(uint64_t id);
// Send GOAWAY if the peer is blocked on the implementation max.
bool OnStreamsBlockedFrame(const QuicStreamsBlockedFrame& frame) override;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index a8e5230..64ef39a 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -631,7 +631,7 @@
testing::InSequence s;
connection_->AdvanceTime(QuicTime::Delta::FromSeconds(1));
GoAwayFrame goaway;
- goaway.stream_id = 0x1;
+ goaway.id = 0x1;
std::unique_ptr<char[]> buffer;
QuicByteCount header_length =
HttpEncoder::SerializeGoAwayFrame(goaway, &buffer);
