Don't use a flow controller for QuicCryptoStream when it uses CRYPTO frames
gfe-relnote: QuicCryptoStream change protected behind QUIC_VERSION_99
PiperOrigin-RevId: 248004335
Change-Id: I97d5b6ed0e5523a4264ce2f06e5f6c3249a99fe9
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index a859f87..85ae602 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -2005,11 +2005,16 @@
auto* server_session = static_cast<QuicSpdySession*>(GetServerSession());
ExpectFlowControlsSynced(client_session->flow_controller(),
server_session->flow_controller());
- ExpectFlowControlsSynced(
- QuicSessionPeer::GetMutableCryptoStream(client_session)
- ->flow_controller(),
- QuicSessionPeer::GetMutableCryptoStream(server_session)
- ->flow_controller());
+ if (!QuicVersionUsesCryptoFrames(client_->client()
+ ->client_session()
+ ->connection()
+ ->transport_version())) {
+ ExpectFlowControlsSynced(
+ QuicSessionPeer::GetMutableCryptoStream(client_session)
+ ->flow_controller(),
+ QuicSessionPeer::GetMutableCryptoStream(server_session)
+ ->flow_controller());
+ }
SpdyFramer spdy_framer(SpdyFramer::ENABLE_COMPRESSION);
SpdySettingsIR settings_frame;
settings_frame.AddSetting(SETTINGS_MAX_HEADER_LIST_SIZE,
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index da920dc..1059a94 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -823,6 +823,12 @@
}
TEST_P(QuicSpdySessionTestServer, BufferedHandshake) {
+ // This tests prioritization of the crypto stream when flow control limits are
+ // reached. When CRYPTO frames are in use, there is no flow control for the
+ // crypto handshake, so this test is irrelevant.
+ if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ return;
+ }
session_.set_writev_consumes_all_data(true);
EXPECT_FALSE(session_.HasPendingHandshake()); // Default value.
@@ -910,8 +916,10 @@
// Mark the crypto and headers streams as write blocked, we expect them to be
// allowed to write later.
- session_.MarkConnectionLevelWriteBlocked(
- QuicUtils::GetCryptoStreamId(connection_->transport_version()));
+ if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ session_.MarkConnectionLevelWriteBlocked(
+ QuicUtils::GetCryptoStreamId(connection_->transport_version()));
+ }
// Create a data stream, and although it is write blocked we never expect it
// to be allowed to write as we are connection level flow control blocked.
@@ -921,10 +929,13 @@
// The crypto and headers streams should be called even though we are
// connection flow control blocked.
- TestCryptoStream* crypto_stream = session_.GetMutableCryptoStream();
- EXPECT_CALL(*crypto_stream, OnCanWrite());
+ if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ TestCryptoStream* crypto_stream = session_.GetMutableCryptoStream();
+ EXPECT_CALL(*crypto_stream, OnCanWrite());
+ }
TestHeadersStream* headers_stream;
- if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+ !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
headers_stream = new TestHeadersStream(&session_);
QuicSpdySessionPeer::SetHeadersStream(&session_, headers_stream);
@@ -1213,6 +1224,11 @@
// various names that are dependent on the parameters passed.
TEST_P(QuicSpdySessionTestServer,
HandshakeUnblocksFlowControlBlockedHeadersStream) {
+ // This test depends on stream-level flow control for the crypto stream, which
+ // doesn't exist when CRYPTO frames are used.
+ if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ return;
+ }
// Test that if the header stream is flow control blocked, then if the SHLO
// contains a larger send window offset, the stream becomes unblocked.
session_.set_writev_consumes_all_data(true);
@@ -1644,7 +1660,8 @@
TEST_P(QuicSpdySessionTestClient, WritePriority) {
TestHeadersStream* headers_stream;
- if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ if (!GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) &&
+ !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QuicSpdySessionPeer::SetHeadersStream(&session_, nullptr);
headers_stream = new TestHeadersStream(&session_);
QuicSpdySessionPeer::SetHeadersStream(&session_, headers_stream);
diff --git a/quic/core/quic_crypto_stream.cc b/quic/core/quic_crypto_stream.cc
index 836fbac..2263546 100644
--- a/quic/core/quic_crypto_stream.cc
+++ b/quic/core/quic_crypto_stream.cc
@@ -28,7 +28,10 @@
session->connection()->transport_version()),
session,
/*is_static=*/true,
- BIDIRECTIONAL),
+ QuicVersionUsesCryptoFrames(
+ session->connection()->transport_version())
+ ? CRYPTO
+ : BIDIRECTIONAL),
substreams_{{this, ENCRYPTION_INITIAL},
{this, ENCRYPTION_HANDSHAKE},
{this, ENCRYPTION_ZERO_RTT},
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index 2505f7e..8bdf3b5 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -86,7 +86,8 @@
closed_streams_clean_up_alarm_(nullptr),
supported_versions_(supported_versions),
eliminate_static_stream_map_(
- GetQuicReloadableFlag(quic_eliminate_static_stream_map_2)) {
+ GetQuicReloadableFlag(quic_eliminate_static_stream_map_2) ||
+ QuicVersionUsesCryptoFrames(connection->transport_version())) {
closed_streams_clean_up_alarm_ =
QuicWrapUnique<QuicAlarm>(connection_->alarm_factory()->CreateAlarm(
new ClosedStreamsCleanUpDelegate(this)));
@@ -1024,7 +1025,8 @@
for (auto const& kv : dynamic_stream_map_) {
kv.second->flow_controller()->UpdateReceiveWindowSize(stream_window);
}
- if (eliminate_static_stream_map_) {
+ if (eliminate_static_stream_map_ &&
+ !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 11, 17);
GetMutableCryptoStream()->flow_controller()->UpdateReceiveWindowSize(
stream_window);
@@ -1074,7 +1076,8 @@
for (auto const& kv : dynamic_stream_map_) {
kv.second->UpdateSendWindowOffset(new_window);
}
- if (eliminate_static_stream_map_) {
+ if (eliminate_static_stream_map_ &&
+ !QuicVersionUsesCryptoFrames(connection_->transport_version())) {
QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 12, 17);
GetMutableCryptoStream()->UpdateSendWindowOffset(new_window);
}
@@ -1454,6 +1457,7 @@
}
}
if (eliminate_static_stream_map_ &&
+ !QuicVersionUsesCryptoFrames(connection_->transport_version()) &&
GetMutableCryptoStream()->flow_controller()->IsBlocked()) {
QUIC_RELOADABLE_FLAG_COUNT_N(quic_eliminate_static_stream_map_2, 14, 17);
return true;
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 1be72f8..a441dc4 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -952,6 +952,11 @@
}
TEST_P(QuicSessionTestServer, BufferedHandshake) {
+ // This test is testing behavior of crypto stream flow control, but when
+ // CRYPTO frames are used, there is no flow control for the crypto handshake.
+ if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ return;
+ }
session_.set_writev_consumes_all_data(true);
EXPECT_FALSE(session_.HasPendingHandshake()); // Default value.
@@ -1038,8 +1043,10 @@
// Mark the crypto and headers streams as write blocked, we expect them to be
// allowed to write later.
- session_.MarkConnectionLevelWriteBlocked(
- QuicUtils::GetCryptoStreamId(connection_->transport_version()));
+ if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ session_.MarkConnectionLevelWriteBlocked(
+ QuicUtils::GetCryptoStreamId(connection_->transport_version()));
+ }
// Create a data stream, and although it is write blocked we never expect it
// to be allowed to write as we are connection level flow control blocked.
@@ -1049,8 +1056,10 @@
// The crypto and headers streams should be called even though we are
// connection flow control blocked.
- TestCryptoStream* crypto_stream = session_.GetMutableCryptoStream();
- EXPECT_CALL(*crypto_stream, OnCanWrite());
+ if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ TestCryptoStream* crypto_stream = session_.GetMutableCryptoStream();
+ EXPECT_CALL(*crypto_stream, OnCanWrite());
+ }
// After the crypto and header streams perform a write, the connection will be
// blocked by the flow control, hence it should become application-limited.
diff --git a/quic/core/quic_stream.cc b/quic/core/quic_stream.cc
index 0d654d6..32b168d 100644
--- a/quic/core/quic_stream.cc
+++ b/quic/core/quic_stream.cc
@@ -181,6 +181,30 @@
sequencer_.set_stream(this);
}
+namespace {
+
+QuicOptional<QuicFlowController> FlowController(QuicStreamId id,
+ QuicSession* session,
+ StreamType type) {
+ if (type == CRYPTO) {
+ // The only QuicStream with a StreamType of CRYPTO is QuicCryptoStream, when
+ // it is using crypto frames instead of stream frames. The QuicCryptoStream
+ // doesn't have any flow control in that case, so we don't create a
+ // QuicFlowController for it.
+ return QuicOptional<QuicFlowController>();
+ }
+ return QuicFlowController(
+ session, id,
+ /*is_connection_flow_controller*/ false,
+ GetReceivedFlowControlWindow(session),
+ GetInitialStreamFlowControlWindowToSend(session),
+ kStreamReceiveWindowLimit,
+ session->flow_controller()->auto_tune_receive_window(),
+ session->flow_controller());
+}
+
+} // namespace
+
QuicStream::QuicStream(QuicStreamId id,
QuicSession* session,
bool is_static,
@@ -192,15 +216,7 @@
type,
0,
false,
- QuicFlowController(
- session,
- id,
- /*is_connection_flow_controller*/ false,
- GetReceivedFlowControlWindow(session),
- GetInitialStreamFlowControlWindowToSend(session),
- kStreamReceiveWindowLimit,
- session->flow_controller()->auto_tune_receive_window(),
- session->flow_controller()),
+ FlowController(id, session, type),
session->flow_controller()) {}
QuicStream::QuicStream(QuicStreamId id,
@@ -210,7 +226,7 @@
StreamType type,
uint64_t stream_bytes_read,
bool fin_received,
- QuicFlowController flow_controller,
+ QuicOptional<QuicFlowController> flow_controller,
QuicFlowController* connection_flow_controller)
: sequencer_(std::move(sequencer)),
id_(id),
@@ -239,7 +255,8 @@
buffered_data_threshold_(GetQuicFlag(FLAGS_quic_buffered_data_threshold)),
is_static_(is_static),
deadline_(QuicTime::Zero()),
- type_(session->connection()->transport_version() == QUIC_VERSION_99
+ type_(session->connection()->transport_version() == QUIC_VERSION_99 &&
+ type != CRYPTO
? QuicUtils::GetStreamType(id_,
perspective_,
session->IsIncomingStream(id_))
@@ -252,7 +269,9 @@
CloseWriteSide();
}
SetFromConfig();
- session_->RegisterStreamPriority(id, is_static_, priority_);
+ if (type_ != CRYPTO) {
+ session_->RegisterStreamPriority(id, is_static_, priority_);
+ }
}
QuicStream::~QuicStream() {
@@ -263,7 +282,7 @@
<< send_buffer_.stream_bytes_outstanding()
<< ", fin_outstanding: " << fin_outstanding_;
}
- if (session_ != nullptr) {
+ if (session_ != nullptr && type_ != CRYPTO) {
session_->UnregisterStreamPriority(id(), is_static_);
}
}
@@ -323,7 +342,7 @@
MaybeIncreaseHighestReceivedOffset(frame.offset + frame_payload_size)) {
// As the highest received offset has changed, check to see if this is a
// violation of flow control.
- if (flow_controller_.FlowControlViolation() ||
+ if (flow_controller_->FlowControlViolation() ||
connection_flow_controller_->FlowControlViolation()) {
CloseConnectionWithDetails(
QUIC_FLOW_CONTROL_RECEIVED_TOO_MUCH_DATA,
@@ -352,7 +371,7 @@
return;
}
MaybeIncreaseHighestReceivedOffset(frame.byte_offset);
- if (flow_controller_.FlowControlViolation() ||
+ if (flow_controller_->FlowControlViolation() ||
connection_flow_controller_->FlowControlViolation()) {
CloseConnectionWithDetails(
QUIC_FLOW_CONTROL_RECEIVED_TOO_MUCH_DATA,
@@ -493,7 +512,7 @@
}
void QuicStream::MaybeSendBlocked() {
- if (flow_controller_.ShouldSendBlocked()) {
+ if (flow_controller_->ShouldSendBlocked()) {
session_->SendBlocked(id_);
}
if (!stream_contributes_to_connection_flow_control_) {
@@ -507,7 +526,7 @@
// the stream will be given a chance to write when a connection-level
// WINDOW_UPDATE arrives.
if (connection_flow_controller_->IsBlocked() &&
- !flow_controller_.IsBlocked()) {
+ !flow_controller_->IsBlocked()) {
session_->MarkConnectionLevelWriteBlocked(id());
}
}
@@ -707,7 +726,7 @@
rst_sent_ = true;
}
- if (flow_controller_.FlowControlViolation() ||
+ if (flow_controller_->FlowControlViolation() ||
connection_flow_controller_->FlowControlViolation()) {
return;
}
@@ -716,13 +735,13 @@
// the same connection level flow control state, mark all unreceived or
// buffered bytes as consumed.
QuicByteCount bytes_to_consume =
- flow_controller_.highest_received_byte_offset() -
- flow_controller_.bytes_consumed();
+ flow_controller_->highest_received_byte_offset() -
+ flow_controller_->bytes_consumed();
AddBytesConsumed(bytes_to_consume);
}
void QuicStream::OnWindowUpdateFrame(const QuicWindowUpdateFrame& frame) {
- if (flow_controller_.UpdateSendWindowOffset(frame.byte_offset)) {
+ if (flow_controller_->UpdateSendWindowOffset(frame.byte_offset)) {
// Let session unblock this stream.
session_->MarkConnectionLevelWriteBlocked(id_);
}
@@ -731,8 +750,8 @@
bool QuicStream::MaybeIncreaseHighestReceivedOffset(
QuicStreamOffset new_offset) {
uint64_t increment =
- new_offset - flow_controller_.highest_received_byte_offset();
- if (!flow_controller_.UpdateHighestReceivedOffset(new_offset)) {
+ new_offset - flow_controller_->highest_received_byte_offset();
+ if (!flow_controller_->UpdateHighestReceivedOffset(new_offset)) {
return false;
}
@@ -748,16 +767,22 @@
}
void QuicStream::AddBytesSent(QuicByteCount bytes) {
- flow_controller_.AddBytesSent(bytes);
+ flow_controller_->AddBytesSent(bytes);
if (stream_contributes_to_connection_flow_control_) {
connection_flow_controller_->AddBytesSent(bytes);
}
}
void QuicStream::AddBytesConsumed(QuicByteCount bytes) {
+ if (type_ == CRYPTO) {
+ // A stream with type CRYPTO has no flow control, so there's nothing this
+ // function needs to do. This function still gets called by the
+ // QuicStreamSequencers used by QuicCryptoStream.
+ return;
+ }
// Only adjust stream level flow controller if still reading.
if (!read_side_closed_) {
- flow_controller_.AddBytesConsumed(bytes);
+ flow_controller_->AddBytesConsumed(bytes);
}
if (stream_contributes_to_connection_flow_control_) {
@@ -766,7 +791,7 @@
}
void QuicStream::UpdateSendWindowOffset(QuicStreamOffset new_window) {
- if (flow_controller_.UpdateSendWindowOffset(new_window)) {
+ if (flow_controller_->UpdateSendWindowOffset(new_window)) {
// Let session unblock this stream.
session_->MarkConnectionLevelWriteBlocked(id_);
}
@@ -919,7 +944,7 @@
bool fin = fin_buffered_;
// How much data flow control permits to be written.
- QuicByteCount send_window = flow_controller_.SendWindowSize();
+ QuicByteCount send_window = flow_controller_->SendWindowSize();
if (stream_contributes_to_connection_flow_control_) {
send_window =
std::min(send_window, connection_flow_controller_->SendWindowSize());
diff --git a/quic/core/quic_stream.h b/quic/core/quic_stream.h
index b6d03de..df1f7f8 100644
--- a/quic/core/quic_stream.h
+++ b/quic/core/quic_stream.h
@@ -30,6 +30,7 @@
#include "net/third_party/quiche/src/quic/core/session_notifier_interface.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_export.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_mem_slice_span.h"
+#include "net/third_party/quiche/src/quic/platform/api/quic_optional.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_reference_counted.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_string_piece.h"
#include "net/third_party/quiche/src/spdy/core/spdy_protocol.h"
@@ -224,7 +225,7 @@
int num_frames_received() const;
int num_duplicate_frames_received() const;
- QuicFlowController* flow_controller() { return &flow_controller_; }
+ QuicFlowController* flow_controller() { return &*flow_controller_; }
// Called when endpoint receives a frame which could increase the highest
// offset.
@@ -433,7 +434,7 @@
StreamType type,
uint64_t stream_bytes_read,
bool fin_received,
- QuicFlowController flow_controller,
+ QuicOptional<QuicFlowController> flow_controller,
QuicFlowController* connection_flow_controller);
// Subclasses and consumers should use reading_stopped.
@@ -504,7 +505,7 @@
// server or a client.
Perspective perspective_;
- QuicFlowController flow_controller_;
+ QuicOptional<QuicFlowController> flow_controller_;
// The connection level flow controller. Not owned.
QuicFlowController* connection_flow_controller_;
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h
index 199ec94..66a90c1 100644
--- a/quic/core/quic_types.h
+++ b/quic/core/quic_types.h
@@ -584,6 +584,9 @@
// Unidirectional streams carry data in one direction only.
WRITE_UNIDIRECTIONAL,
READ_UNIDIRECTIONAL,
+ // Not actually a stream type. Used only by QuicCryptoStream when it uses
+ // CRYPTO frames and isn't actually a QuicStream.
+ CRYPTO,
};
// A packet number space is the context in which a packet can be processed and
diff --git a/quic/platform/api/quic_optional.h b/quic/platform/api/quic_optional.h
new file mode 100644
index 0000000..921c3a8
--- /dev/null
+++ b/quic/platform/api/quic_optional.h
@@ -0,0 +1,17 @@
+// Copyright (c) 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef QUICHE_QUIC_PLATFORM_API_QUIC_OPTIONAL_H_
+#define QUICHE_QUIC_PLATFORM_API_QUIC_OPTIONAL_H_
+
+#include "net/quic/platform/impl/quic_optional_impl.h"
+
+namespace quic {
+
+template <typename T>
+using QuicOptional = QuicOptionalImpl<T>;
+
+} // namespace quic
+
+#endif // QUICHE_QUIC_PLATFORM_API_QUIC_OPTIONAL_H_
