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_