diff options
author | rtenneti <rtenneti@chromium.org> | 2014-12-04 14:57:58 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-12-04 22:58:31 +0000 |
commit | 89101772304452c3da761dfadc510a3aac829c82 (patch) | |
tree | 375c65a4b5e2be07c61ff0b0ef575888e6740e4c | |
parent | ee38e1d7cea77b253b0f30bf931e20a9da09f76e (diff) | |
download | chromium_src-89101772304452c3da761dfadc510a3aac829c82.zip chromium_src-89101772304452c3da761dfadc510a3aac829c82.tar.gz chromium_src-89101772304452c3da761dfadc510a3aac829c82.tar.bz2 |
Land Recent QUIC Changes
QUIC connection stability prober - added goaway_received API call.
Merge internal change: 81360468
https://codereview.chromium.org/740843004/
QUIC - minor changes to fix formatting.
Merge internal change: 81347909
https://codereview.chromium.org/779133002/
Add the option to not send an explicit QuicConnectionClose frame when
the reason is QUIC_CONNECTION_TIMED_OUT.
This saves bandwidth and most importantly prevents the radio from being
woken up to send a useless packet on mobile clients.
Merge internal change: 81247453
https://codereview.chromium.org/761793003/
Remove QUIC's optional KATO config param, because it was never used.
Merge internal change: 81198447
https://codereview.chromium.org/750093008/
Added flag for enabling pacing for the command-line options for internal
servers.
When false, pacing may still be configured via config options or by
turning on BBR. When true, turns on pacing unconditionally. This
flag is intended to be used for performance testing.
Adds QUIC flag FLAGS_quic_enable_pacing for enabling pacing.
Merge internal change: 81166053
https://codereview.chromium.org/777273002/
Only send a server config update when there is no other data to write.
Merge internal change: 81147141
https://codereview.chromium.org/781853002/
Removing deprecated flag FLAGS_quic_drop_junk_packets
Merge internal change: 81092917
https://codereview.chromium.org/748803007/
Merging of rollback of internal changelist 79416786.
Rollback chromium CL: https://codereview.chromium.org/731593002/
Prevents XTs from dying on restart.
*** Reason for rollback ***
Possible corrupted pointer dereference in CL 78946498 must be rolled back.
*** Original change description ***
Fix QUIC end_to_end_test flakiness introduced by CL 78946498
N/A. Should affect tests only.
CL 78946498 introduced some new tests for the "Small Red Button", a
flag that, when enabled, forces QUIC to disconnect all active sessions
and start rejecting traffic.
Problem:
The new CL made the en...
***
Merge internal change: 81066584
https://codereview.chromium.org/779463006/
QUIC - Use uint16 for ports.
No functionality change. Changes to keep the code similar to internal
source tree.
Merge internal change: 80778883
https://codereview.chromium.org/742513009/
R=rch@chromium.org
Review URL: https://codereview.chromium.org/780123002
Cr-Commit-Position: refs/heads/master@{#306921}
26 files changed, 214 insertions, 54 deletions
diff --git a/net/quic/crypto/crypto_handshake_message.cc b/net/quic/crypto/crypto_handshake_message.cc index 23e8ea2..6ed8014 100644 --- a/net/quic/crypto/crypto_handshake_message.cc +++ b/net/quic/crypto/crypto_handshake_message.cc @@ -244,7 +244,6 @@ string CryptoHandshakeMessage::DebugStringInternal(size_t indent) const { case kCFCW: case kSFCW: case kIRTT: - case kKATO: case kMSPC: case kSWND: // uint32 value diff --git a/net/quic/crypto/crypto_protocol.h b/net/quic/crypto/crypto_protocol.h index 57a5508..a9337a5 100644 --- a/net/quic/crypto/crypto_protocol.h +++ b/net/quic/crypto/crypto_protocol.h @@ -94,7 +94,7 @@ const QuicTag kCGST = TAG('C', 'G', 'S', 'T'); // Congestion control const QuicTag kCOPT = TAG('C', 'O', 'P', 'T'); // Connection options const QuicTag kICSL = TAG('I', 'C', 'S', 'L'); // Idle connection state // lifetime -const QuicTag kKATO = TAG('K', 'A', 'T', 'O'); // Keepalive timeout +const QuicTag kSCLS = TAG('S', 'C', 'L', 'S'); // Silently close on timeout const QuicTag kMSPC = TAG('M', 'S', 'P', 'C'); // Max streams per connection. const QuicTag kIRTT = TAG('I', 'R', 'T', 'T'); // Estimated initial RTT in us. const QuicTag kSWND = TAG('S', 'W', 'N', 'D'); // Server's Initial congestion diff --git a/net/quic/quic_config.cc b/net/quic/quic_config.cc index d2a10a4..bb686e7 100644 --- a/net/quic/quic_config.cc +++ b/net/quic/quic_config.cc @@ -431,7 +431,7 @@ QuicConfig::QuicConfig() congestion_feedback_(kCGST, PRESENCE_REQUIRED), connection_options_(kCOPT, PRESENCE_OPTIONAL), idle_connection_state_lifetime_seconds_(kICSL, PRESENCE_REQUIRED), - keepalive_timeout_seconds_(kKATO, PRESENCE_OPTIONAL), + silent_close_(kSCLS, PRESENCE_OPTIONAL), max_streams_per_connection_(kMSPC, PRESENCE_REQUIRED), bytes_for_connection_id_(kTCID, PRESENCE_OPTIONAL), initial_round_trip_time_us_(kIRTT, PRESENCE_OPTIONAL), @@ -493,9 +493,12 @@ QuicTime::Delta QuicConfig::IdleConnectionStateLifetime() const { idle_connection_state_lifetime_seconds_.GetUint32()); } -QuicTime::Delta QuicConfig::KeepaliveTimeout() const { - return QuicTime::Delta::FromSeconds( - keepalive_timeout_seconds_.GetUint32()); +void QuicConfig::SetSilentClose(bool silent_close) { + silent_close_.set(silent_close ? 1 : 0, silent_close ? 1 : 0); +} + +bool QuicConfig::SilentClose() const { + return silent_close_.GetUint32() > 0; } void QuicConfig::SetMaxStreamsPerConnection(size_t max_streams, @@ -631,7 +634,6 @@ bool QuicConfig::negotiated() const { // ProcessServerHello. return congestion_feedback_.negotiated() && idle_connection_state_lifetime_seconds_.negotiated() && - keepalive_timeout_seconds_.negotiated() && max_streams_per_connection_.negotiated(); } @@ -641,8 +643,11 @@ void QuicConfig::SetDefaults() { congestion_feedback_.set(congestion_feedback, kQBIC); idle_connection_state_lifetime_seconds_.set(kMaximumIdleTimeoutSecs, kDefaultIdleTimeoutSecs); - // kKATO is optional. Return 0 if not negotiated. - keepalive_timeout_seconds_.set(0, 0); + if (FLAGS_quic_allow_silent_close) { + silent_close_.set(1, 0); + } else { + silent_close_.set(0, 0); + } SetMaxStreamsPerConnection(kDefaultMaxStreamsPerConnection, kDefaultMaxStreamsPerConnection); max_time_before_crypto_handshake_ = @@ -659,7 +664,7 @@ void QuicConfig::SetDefaults() { void QuicConfig::ToHandshakeMessage(CryptoHandshakeMessage* out) const { congestion_feedback_.ToHandshakeMessage(out); idle_connection_state_lifetime_seconds_.ToHandshakeMessage(out); - keepalive_timeout_seconds_.ToHandshakeMessage(out); + silent_close_.ToHandshakeMessage(out); max_streams_per_connection_.ToHandshakeMessage(out); bytes_for_connection_id_.ToHandshakeMessage(out); initial_round_trip_time_us_.ToHandshakeMessage(out); @@ -686,8 +691,8 @@ QuicErrorCode QuicConfig::ProcessPeerHello( peer_hello, hello_type, error_details); } if (error == QUIC_NO_ERROR) { - error = keepalive_timeout_seconds_.ProcessPeerHello( - peer_hello, hello_type, error_details); + error = + silent_close_.ProcessPeerHello(peer_hello, hello_type, error_details); } if (error == QUIC_NO_ERROR) { error = max_streams_per_connection_.ProcessPeerHello( diff --git a/net/quic/quic_config.h b/net/quic/quic_config.h index 507dd9d..12097f8 100644 --- a/net/quic/quic_config.h +++ b/net/quic/quic_config.h @@ -277,7 +277,9 @@ class NET_EXPORT_PRIVATE QuicConfig { QuicTime::Delta IdleConnectionStateLifetime() const; - QuicTime::Delta KeepaliveTimeout() const; + void SetSilentClose(bool silent_close); + + bool SilentClose() const; void SetMaxStreamsPerConnection(size_t max_streams, size_t default_streams); @@ -400,8 +402,8 @@ class NET_EXPORT_PRIVATE QuicConfig { QuicFixedTagVector connection_options_; // Idle connection state lifetime QuicNegotiableUint32 idle_connection_state_lifetime_seconds_; - // Keepalive timeout, or 0 to turn off keepalive probes - QuicNegotiableUint32 keepalive_timeout_seconds_; + // Whether to use silent close. Defaults to 0 (false) and is otherwise true. + QuicNegotiableUint32 silent_close_; // Maximum number of streams that the connection can support. QuicNegotiableUint32 max_streams_per_connection_; // The number of bytes required for the connection ID. diff --git a/net/quic/quic_config_test.cc b/net/quic/quic_config_test.cc index 413d7fb..36321d1 100644 --- a/net/quic/quic_config_test.cc +++ b/net/quic/quic_config_test.cc @@ -95,6 +95,7 @@ TEST_F(QuicConfigTest, ProcessClientHello) { client_config.SetConnectionOptionsToSend(copt); CryptoHandshakeMessage msg; client_config.ToHandshakeMessage(&msg); + string error_details; const QuicErrorCode error = config_.ProcessPeerHello(msg, CLIENT, &error_details); @@ -105,7 +106,6 @@ TEST_F(QuicConfigTest, ProcessClientHello) { config_.IdleConnectionStateLifetime()); EXPECT_EQ(kDefaultMaxStreamsPerConnection, config_.MaxStreamsPerConnection()); - EXPECT_EQ(QuicTime::Delta::FromSeconds(0), config_.KeepaliveTimeout()); EXPECT_EQ(10 * kNumMicrosPerMilli, config_.ReceivedInitialRoundTripTimeUs()); EXPECT_TRUE(config_.HasReceivedConnectionOptions()); EXPECT_EQ(2u, config_.ReceivedConnectionOptions().size()); @@ -152,7 +152,6 @@ TEST_F(QuicConfigTest, ProcessServerHello) { config_.IdleConnectionStateLifetime()); EXPECT_EQ(kDefaultMaxStreamsPerConnection / 2, config_.MaxStreamsPerConnection()); - EXPECT_EQ(QuicTime::Delta::FromSeconds(0), config_.KeepaliveTimeout()); EXPECT_EQ(10 * kNumMicrosPerMilli, config_.ReceivedInitialRoundTripTimeUs()); EXPECT_EQ(config_.ReceivedInitialFlowControlWindowBytes(), 2 * kInitialSessionFlowControlWindowForTest); @@ -179,6 +178,7 @@ TEST_F(QuicConfigTest, MissingOptionalValuesInCHLO) { const QuicErrorCode error = config_.ProcessPeerHello(msg, CLIENT, &error_details); EXPECT_EQ(QUIC_NO_ERROR, error); + EXPECT_TRUE(config_.negotiated()); EXPECT_FALSE(config_.HasReceivedInitialFlowControlWindowBytes()); } @@ -196,6 +196,7 @@ TEST_F(QuicConfigTest, MissingOptionalValuesInSHLO) { const QuicErrorCode error = config_.ProcessPeerHello(msg, SERVER, &error_details); EXPECT_EQ(QUIC_NO_ERROR, error); + EXPECT_TRUE(config_.negotiated()); EXPECT_FALSE(config_.HasReceivedInitialFlowControlWindowBytes()); } diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc index c33aec5..74d1758 100644 --- a/net/quic/quic_connection.cc +++ b/net/quic/quic_connection.cc @@ -192,7 +192,8 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id, bool is_server, bool is_secure, const QuicVersionVector& supported_versions) - : framer_(supported_versions, helper->GetClock()->ApproximateNow(), + : framer_(supported_versions, + helper->GetClock()->ApproximateNow(), is_server), helper_(helper), writer_(writer_factory.Create(this)), @@ -213,6 +214,7 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id, largest_seen_packet_with_stop_waiting_(0), max_undecryptable_packets_(0), pending_version_negotiation_packet_(false), + silent_close_enabled_(false), received_packet_manager_(&stats_), ack_queued_(false), num_packets_received_since_last_ack_sent_(0), @@ -230,7 +232,9 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id, time_of_last_sent_new_packet_(clock_->ApproximateNow()), sequence_number_of_last_sent_packet_(0), sent_packet_manager_( - is_server, clock_, &stats_, + is_server, + clock_, + &stats_, FLAGS_quic_use_bbr_congestion_control ? kBBR : kCubic, FLAGS_quic_use_time_loss_detection ? kTime : kNack, is_secure), @@ -268,6 +272,9 @@ void QuicConnection::SetFromConfig(const QuicConfig& config) { if (config.negotiated()) { SetNetworkTimeouts(QuicTime::Delta::Infinite(), config.IdleConnectionStateLifetime()); + if (FLAGS_quic_allow_silent_close && config.SilentClose()) { + silent_close_enabled_ = true; + } } else { SetNetworkTimeouts(config.max_time_before_crypto_handshake(), config.max_idle_time_before_crypto_handshake()); @@ -313,14 +320,8 @@ bool QuicConnection::SelectMutualVersion( void QuicConnection::OnError(QuicFramer* framer) { // Packets that we can not or have not decrypted are dropped. // TODO(rch): add stats to measure this. - if (FLAGS_quic_drop_junk_packets) { - if (!connected_ || last_packet_decrypted_ == false) { - return; - } - } else { - if (!connected_ || framer->error() == QUIC_DECRYPTION_FAILURE) { - return; - } + if (!connected_ || last_packet_decrypted_ == false) { + return; } SendConnectionCloseWithDetails(framer->error(), framer->detailed_error()); } @@ -1854,6 +1855,12 @@ void QuicConnection::SendConnectionClosePacket(QuicErrorCode error, DVLOG(1) << ENDPOINT << "Force closing " << connection_id() << " with error " << QuicUtils::ErrorToString(error) << " (" << error << ") " << details; + // Don't send explicit connection close packets for timeouts. + // This is particularly important on mobile, where connections are short. + if (silent_close_enabled_ && + error == QuicErrorCode::QUIC_CONNECTION_TIMED_OUT) { + return; + } ScopedPacketBundler ack_bundler(this, SEND_ACK); QuicConnectionCloseFrame* frame = new QuicConnectionCloseFrame(); frame->error_code = error; diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h index 14c45a1..76dabff 100644 --- a/net/quic/quic_connection.h +++ b/net/quic/quic_connection.h @@ -745,6 +745,9 @@ class NET_EXPORT_PRIVATE QuicConnection // Contains the connection close packet if the connection has been closed. scoped_ptr<QuicEncryptedPacket> connection_close_packet_; + // When true, the connection does not send a close packet on timeout. + bool silent_close_enabled_; + FecGroupMap group_map_; QuicReceivedPacketManager received_packet_manager_; diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc index 32d7544..9d9a506 100644 --- a/net/quic/quic_connection_test.cc +++ b/net/quic/quic_connection_test.cc @@ -2987,6 +2987,7 @@ TEST_P(QuicConnectionTest, TimeoutAfterSend) { EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _, _)); QuicConfig config; connection_.SetFromConfig(config); + EXPECT_FALSE(QuicConnectionPeer::IsSilentCloseEnabled(&connection_)); const QuicTime::Delta initial_idle_timeout = QuicTime::Delta::FromSeconds(kInitialIdleTimeoutSecs - 1); @@ -3021,6 +3022,67 @@ TEST_P(QuicConnectionTest, TimeoutAfterSend) { EXPECT_FALSE(connection_.connected()); } +TEST_P(QuicConnectionTest, TimeoutAfterSendSilentClose) { + // Same test as above, but complete a handshake which enables silent close, + // causing no connection close packet to be sent. + ValueRestore<bool> old_flag(&FLAGS_quic_allow_silent_close, true); + EXPECT_TRUE(connection_.connected()); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _, _)); + QuicConfig config; + + // Create a handshake message that also enables silent close. + CryptoHandshakeMessage msg; + string error_details; + QuicConfig client_config; + client_config.SetInitialFlowControlWindowToSend( + kInitialSessionFlowControlWindowForTest); + client_config.SetInitialStreamFlowControlWindowToSend( + kInitialStreamFlowControlWindowForTest); + client_config.SetInitialSessionFlowControlWindowToSend( + kInitialSessionFlowControlWindowForTest); + client_config.SetIdleConnectionStateLifetime( + QuicTime::Delta::FromSeconds(kDefaultIdleTimeoutSecs), + QuicTime::Delta::FromSeconds(kDefaultIdleTimeoutSecs)); + client_config.ToHandshakeMessage(&msg); + const QuicErrorCode error = + config.ProcessPeerHello(msg, CLIENT, &error_details); + EXPECT_EQ(QUIC_NO_ERROR, error); + + connection_.SetFromConfig(config); + EXPECT_TRUE(QuicConnectionPeer::IsSilentCloseEnabled(&connection_)); + + const QuicTime::Delta default_idle_timeout = + QuicTime::Delta::FromSeconds(kDefaultIdleTimeoutSecs - 1); + const QuicTime::Delta five_ms = QuicTime::Delta::FromMilliseconds(5); + QuicTime default_timeout = clock_.ApproximateNow().Add(default_idle_timeout); + + // When we send a packet, the timeout will change to 5ms + + // kInitialIdleTimeoutSecs. + clock_.AdvanceTime(five_ms); + + // Send an ack so we don't set the retransmission alarm. + SendAckPacketToPeer(); + EXPECT_EQ(default_timeout, connection_.GetTimeoutAlarm()->deadline()); + + // The original alarm will fire. We should not time out because we had a + // network event at t=5ms. The alarm will reregister. + clock_.AdvanceTime(default_idle_timeout.Subtract(five_ms)); + EXPECT_EQ(default_timeout, clock_.ApproximateNow()); + connection_.GetTimeoutAlarm()->Fire(); + EXPECT_TRUE(connection_.GetTimeoutAlarm()->IsSet()); + EXPECT_TRUE(connection_.connected()); + EXPECT_EQ(default_timeout.Add(five_ms), + connection_.GetTimeoutAlarm()->deadline()); + + // This time, we should time out. + EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_CONNECTION_TIMED_OUT, false)); + clock_.AdvanceTime(five_ms); + EXPECT_EQ(default_timeout.Add(five_ms), clock_.ApproximateNow()); + connection_.GetTimeoutAlarm()->Fire(); + EXPECT_FALSE(connection_.GetTimeoutAlarm()->IsSet()); + EXPECT_FALSE(connection_.connected()); +} + TEST_P(QuicConnectionTest, SendScheduler) { // Test that if we send a packet without delay, it is not queued. QuicPacket* packet = ConstructDataPacket(1, 0, !kEntropyFlag); diff --git a/net/quic/quic_crypto_client_stream_test.cc b/net/quic/quic_crypto_client_stream_test.cc index 8e14d77..a39df4f 100644 --- a/net/quic/quic_crypto_client_stream_test.cc +++ b/net/quic/quic_crypto_client_stream_test.cc @@ -98,7 +98,6 @@ TEST_F(QuicCryptoClientStreamTest, NegotiatedParameters) { config->IdleConnectionStateLifetime().ToSeconds()); EXPECT_EQ(kDefaultMaxStreamsPerConnection, config->MaxStreamsPerConnection()); - EXPECT_EQ(0, config->KeepaliveTimeout().ToSeconds()); const QuicCryptoNegotiatedParameters& crypto_params( stream_->crypto_negotiated_params()); diff --git a/net/quic/quic_flags.cc b/net/quic/quic_flags.cc index c9658c1..306ac2d 100644 --- a/net/quic/quic_flags.cc +++ b/net/quic/quic_flags.cc @@ -32,10 +32,6 @@ bool FLAGS_enable_quic_fec = false; // When true, defaults to BBR congestion control instead of Cubic. bool FLAGS_quic_use_bbr_congestion_control = false; -// If true, QUIC will be more resilliant to junk packets with valid connection -// IDs. -bool FLAGS_quic_drop_junk_packets = true; - // If true, QUIC BBR congestion control may be enabled via Finch and/or via QUIC // connection options. bool FLAGS_quic_allow_bbr = false; @@ -61,3 +57,11 @@ bool FLAGS_quic_record_send_time_before_write = false; // If true, enables the QUIC bandwidth resumption experiment (triggered by // Chrome/Finch). bool FLAGS_quic_enable_bandwidth_resumption_experiment = true; + +// If true, QUIC congestion control will be paced. If false, pacing may be +// controlled by QUIC connection options in the config or by enabling BBR +// congestion control. +bool FLAGS_quic_enable_pacing = false; + +// If true, the silent close option will be honored. +bool FLAGS_quic_allow_silent_close = true; diff --git a/net/quic/quic_flags.h b/net/quic/quic_flags.h index 32c5edf..05e939e 100644 --- a/net/quic/quic_flags.h +++ b/net/quic/quic_flags.h @@ -14,7 +14,6 @@ NET_EXPORT_PRIVATE extern bool FLAGS_use_early_return_when_verifying_chlo; NET_EXPORT_PRIVATE extern bool FLAGS_send_quic_crypto_reject_reason; NET_EXPORT_PRIVATE extern bool FLAGS_enable_quic_fec; NET_EXPORT_PRIVATE extern bool FLAGS_quic_use_bbr_congestion_control; -NET_EXPORT_PRIVATE extern bool FLAGS_quic_drop_junk_packets; NET_EXPORT_PRIVATE extern bool FLAGS_quic_allow_bbr; NET_EXPORT_PRIVATE extern bool FLAGS_allow_truncated_connection_ids_for_quic; NET_EXPORT_PRIVATE extern bool FLAGS_quic_too_many_outstanding_packets; @@ -22,5 +21,7 @@ NET_EXPORT_PRIVATE extern bool FLAGS_enable_quic_delay_forward_security; NET_EXPORT_PRIVATE extern bool FLAGS_quic_record_send_time_before_write; NET_EXPORT_PRIVATE extern bool FLAGS_quic_enable_bandwidth_resumption_experiment; +NET_EXPORT_PRIVATE extern bool FLAGS_quic_enable_pacing; +NET_EXPORT_PRIVATE extern bool FLAGS_quic_allow_silent_close; #endif // NET_QUIC_QUIC_FLAGS_H_ diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc index 56c327c..9f3f3d3 100644 --- a/net/quic/quic_sent_packet_manager.cc +++ b/net/quic/quic_sent_packet_manager.cc @@ -134,8 +134,8 @@ void QuicSentPacketManager::SetFromConfig(const QuicConfig& config) { clock_, &rtt_stats_, kReno, stats_, initial_congestion_window_)); } if (HasClientSentConnectionOption(config, kPACE) || - (FLAGS_quic_allow_bbr && - HasClientSentConnectionOption(config, kTBBR))) { + FLAGS_quic_enable_pacing || + (FLAGS_quic_allow_bbr && HasClientSentConnectionOption(config, kTBBR))) { EnablePacing(); } if (HasClientSentConnectionOption(config, k1CON)) { diff --git a/net/quic/quic_sent_packet_manager_test.cc b/net/quic/quic_sent_packet_manager_test.cc index c1e6f20..1a907ff 100644 --- a/net/quic/quic_sent_packet_manager_test.cc +++ b/net/quic/quic_sent_packet_manager_test.cc @@ -1384,6 +1384,20 @@ TEST_F(QuicSentPacketManagerTest, NegotiatePacingFromOptions) { EXPECT_TRUE(manager_.using_pacing()); } +TEST_F(QuicSentPacketManagerTest, EnablePacingViaFlag) { + EXPECT_FALSE(manager_.using_pacing()); + + // If pacing is enabled via command-line flag, it will be turned on, + // regardless of the contents of the config. + ValueRestore<bool> old_flag(&FLAGS_quic_enable_pacing, true); + QuicConfig config; + EXPECT_CALL(*network_change_visitor_, OnCongestionWindowChange()); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _, /* using_pacing= */ true)); + manager_.SetFromConfig(config); + + EXPECT_TRUE(manager_.using_pacing()); +} + TEST_F(QuicSentPacketManagerTest, NegotiateReceiveWindowFromOptions) { EXPECT_EQ(kDefaultSocketReceiveBuffer, QuicSentPacketManagerPeer::GetReceiveWindow(&manager_)); diff --git a/net/quic/quic_server_session.cc b/net/quic/quic_server_session.cc index e0defd6..64857c0 100644 --- a/net/quic/quic_server_session.cc +++ b/net/quic/quic_server_session.cc @@ -83,6 +83,11 @@ void QuicServerSession::OnCongestionWindowChange(QuicTime now) { return; } + // Only send updates when the application has no data to write. + if (HasDataToWrite()) { + return; + } + // If not enough time has passed since the last time we sent an update to the // client, or not enough packets have been sent, then return early. const QuicSentPacketManager& sent_packet_manager = diff --git a/net/quic/quic_time_wait_list_manager.cc b/net/quic/quic_time_wait_list_manager.cc index 67d347f..8f1f5641 100644 --- a/net/quic/quic_time_wait_list_manager.cc +++ b/net/quic/quic_time_wait_list_manager.cc @@ -27,8 +27,9 @@ namespace net { namespace { -// Time period for which the connection_id should live in time wait state. -const int kTimeWaitSeconds = 5; +// Time period for which a given connection_id should live in the time-wait +// state. +int64 FLAGS_quic_time_wait_list_seconds = 5; } // namespace @@ -88,7 +89,8 @@ QuicTimeWaitListManager::QuicTimeWaitListManager( QuicConnectionHelperInterface* helper, const QuicVersionVector& supported_versions) : helper_(helper), - kTimeWaitPeriod_(QuicTime::Delta::FromSeconds(kTimeWaitSeconds)), + kTimeWaitPeriod_( + QuicTime::Delta::FromSeconds(FLAGS_quic_time_wait_list_seconds)), connection_id_clean_up_alarm_( helper_->CreateAlarm(new ConnectionIdCleanUpAlarm(this))), writer_(writer), @@ -276,6 +278,7 @@ void QuicTimeWaitListManager::CleanUpOldConnectionIds() { break; } // This connection_id has lived its age, retire it now. + DVLOG(1) << "Retiring " << it->first << " from the time-wait state."; delete it->second.close_packet; connection_id_map_.erase(it); } diff --git a/net/quic/test_tools/quic_connection_peer.cc b/net/quic/test_tools/quic_connection_peer.cc index 8a8df46..f986d2e 100644 --- a/net/quic/test_tools/quic_connection_peer.cc +++ b/net/quic/test_tools/quic_connection_peer.cc @@ -154,6 +154,11 @@ void QuicConnectionPeer::SetPeerAddress(QuicConnection* connection, } // static +bool QuicConnectionPeer::IsSilentCloseEnabled(QuicConnection* connection) { + return connection->silent_close_enabled_; +} + +// static void QuicConnectionPeer::SwapCrypters(QuicConnection* connection, QuicFramer* framer) { QuicFramerPeer::SwapCrypters(framer, &connection->framer_); diff --git a/net/quic/test_tools/quic_connection_peer.h b/net/quic/test_tools/quic_connection_peer.h index d452d43..268b139 100644 --- a/net/quic/test_tools/quic_connection_peer.h +++ b/net/quic/test_tools/quic_connection_peer.h @@ -91,6 +91,8 @@ class QuicConnectionPeer { static void SetPeerAddress(QuicConnection* connection, const IPEndPoint& peer_address); + static bool IsSilentCloseEnabled(QuicConnection* connection); + static void SwapCrypters(QuicConnection* connection, QuicFramer* framer); static QuicConnectionHelperInterface* GetHelper(QuicConnection* connection); diff --git a/net/tools/quic/end_to_end_test.cc b/net/tools/quic/end_to_end_test.cc index f7efc71..0cde019 100644 --- a/net/tools/quic/end_to_end_test.cc +++ b/net/tools/quic/end_to_end_test.cc @@ -1347,6 +1347,28 @@ TEST_P(EndToEndTest, RequestWithNoBodyWillNeverSendStreamFrameWithFIN) { server_thread_->Resume(); } +TEST_P(EndToEndTest, EnablePacingViaFlag) { + // When pacing is enabled via command-line flag, it will always be enabled, + // regardless of the config. or the specific congestion-control algorithm. + ValueRestore<bool> old_flag(&FLAGS_quic_enable_pacing, true); + ASSERT_TRUE(Initialize()); + + client_->client()->WaitForCryptoHandshakeConfirmed(); + server_thread_->WaitForCryptoHandshakeConfirmed(); + + // Pause the server so we can access the server's internals without races. + server_thread_->Pause(); + QuicDispatcher* dispatcher = + QuicServerPeer::GetDispatcher(server_thread_->server()); + ASSERT_EQ(1u, dispatcher->session_map().size()); + const QuicSentPacketManager& client_sent_packet_manager = + client_->client()->session()->connection()->sent_packet_manager(); + const QuicSentPacketManager& server_sent_packet_manager = + *GetSentPacketManagerFromFirstServerSession(); + EXPECT_TRUE(server_sent_packet_manager.using_pacing()); + EXPECT_TRUE(client_sent_packet_manager.using_pacing()); +} + } // namespace } // namespace test } // namespace tools diff --git a/net/tools/quic/quic_client.cc b/net/tools/quic/quic_client.cc index 74874d7..6264be0 100644 --- a/net/tools/quic/quic_client.cc +++ b/net/tools/quic/quic_client.cc @@ -235,6 +235,10 @@ void QuicClient::SendRequestsAndWaitForResponse( headers.SetRequestFirstlineFromStringPieces("GET", args[i], "HTTP/1.1"); QuicSpdyClientStream* stream = CreateReliableClientStream(); DCHECK(stream != nullptr); + if (stream == nullptr) { + LOG(ERROR) << "stream creation failed!"; + break; + } stream->SendRequest(headers, "", true); stream->set_visitor(this); } @@ -317,6 +321,10 @@ bool QuicClient::connected() const { session_->connection()->connected(); } +bool QuicClient::goaway_received() const { + return session_ != nullptr && session_->goaway_received(); +} + QuicConnectionId QuicClient::GenerateConnectionId() { return QuicRandom::GetInstance()->RandUint64(); } diff --git a/net/tools/quic/quic_client.h b/net/tools/quic/quic_client.h index 6bf87f9..b983119c 100644 --- a/net/tools/quic/quic_client.h +++ b/net/tools/quic/quic_client.h @@ -120,6 +120,7 @@ class QuicClient : public EpollCallbackInterface, QuicClientSession* session() { return session_.get(); } bool connected() const; + bool goaway_received() const; void set_bind_to_address(IPAddressNumber address) { bind_to_address_ = address; diff --git a/net/tools/quic/quic_server_session.cc b/net/tools/quic/quic_server_session.cc index 9db2365..6e64da5 100644 --- a/net/tools/quic/quic_server_session.cc +++ b/net/tools/quic/quic_server_session.cc @@ -84,6 +84,11 @@ void QuicServerSession::OnCongestionWindowChange(QuicTime now) { return; } + // Only send updates when the application has no data to write. + if (HasDataToWrite()) { + return; + } + // If not enough time has passed since the last time we sent an update to the // client, or not enough packets have been sent, then return early. const QuicSentPacketManager& sent_packet_manager = diff --git a/net/tools/quic/quic_server_session_test.cc b/net/tools/quic/quic_server_session_test.cc index 4705c05..94ed5d8 100644 --- a/net/tools/quic/quic_server_session_test.cc +++ b/net/tools/quic/quic_server_session_test.cc @@ -305,7 +305,8 @@ TEST_P(QuicServerSessionTest, BandwidthEstimates) { } // Test that bandwidth estimate updates are sent to the client, only after the - // bandwidth estimate has changes sufficiently, and enough time has passed. + // bandwidth estimate has changes sufficiently, and enough time has passed, + // and we don't have any other data to write. int32 bandwidth_estimate_kbytes_per_second = 123; int32 max_bandwidth_estimate_kbytes_per_second = 134; @@ -332,6 +333,10 @@ TEST_P(QuicServerSessionTest, BandwidthEstimates) { QuicSustainedBandwidthRecorderPeer::SetMaxBandwidthEstimate( &bandwidth_recorder, max_bandwidth_estimate_kbytes_per_second, max_bandwidth_estimate_timestamp); + // Queue up some pending data. + session_->MarkWriteBlocked(kCryptoStreamId, + QuicWriteBlockedList::kHighestPriority); + EXPECT_TRUE(session_->HasDataToWrite()); // There will be no update sent yet - not enough time has passed. QuicTime now = QuicTime::Zero(); @@ -351,6 +356,11 @@ TEST_P(QuicServerSessionTest, BandwidthEstimates) { kMinIntervalBetweenServerConfigUpdatesRTTs * srtt_ms)); session_->OnCongestionWindowChange(now); + // The connection no longer has pending data to be written. + session_->OnCanWrite(); + EXPECT_FALSE(session_->HasDataToWrite()); + session_->OnCongestionWindowChange(now); + // Bandwidth estimate has now changed sufficiently, enough time has passed, // and enough packets have been sent. QuicConnectionPeer::SetSequenceNumberOfLastSentPacket( diff --git a/net/tools/quic/quic_socket_utils.cc b/net/tools/quic/quic_socket_utils.cc index 3ac0bf3..0eb1185 100644 --- a/net/tools/quic/quic_socket_utils.cc +++ b/net/tools/quic/quic_socket_utils.cc @@ -23,7 +23,7 @@ namespace net { namespace tools { // static -IPAddressNumber QuicSocketUtils::GetAddressFromMsghdr(struct msghdr *hdr) { +IPAddressNumber QuicSocketUtils::GetAddressFromMsghdr(struct msghdr* hdr) { if (hdr->msg_controllen > 0) { for (cmsghdr* cmsg = CMSG_FIRSTHDR(hdr); cmsg != nullptr; @@ -51,11 +51,10 @@ IPAddressNumber QuicSocketUtils::GetAddressFromMsghdr(struct msghdr *hdr) { } // static -bool QuicSocketUtils::GetOverflowFromMsghdr( - struct msghdr *hdr, - QuicPacketCount *dropped_packets) { +bool QuicSocketUtils::GetOverflowFromMsghdr(struct msghdr* hdr, + QuicPacketCount* dropped_packets) { if (hdr->msg_controllen > 0) { - struct cmsghdr *cmsg; + struct cmsghdr* cmsg; for (cmsg = CMSG_FIRSTHDR(hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(hdr, cmsg)) { @@ -103,7 +102,7 @@ int QuicSocketUtils::ReadPacket(int fd, char* buffer, size_t buf_len, QuicPacketCount* dropped_packets, IPAddressNumber* self_address, IPEndPoint* peer_address) { - CHECK(peer_address != nullptr); + DCHECK(peer_address != nullptr); const int kSpaceForOverflowAndIp = CMSG_SPACE(sizeof(int)) + CMSG_SPACE(sizeof(in6_pktinfo)); char cbuf[kSpaceForOverflowAndIp]; @@ -119,7 +118,7 @@ int QuicSocketUtils::ReadPacket(int fd, char* buffer, size_t buf_len, hdr.msg_iovlen = 1; hdr.msg_flags = 0; - struct cmsghdr *cmsg = (struct cmsghdr *) cbuf; + struct cmsghdr* cmsg = (struct cmsghdr*)cbuf; cmsg->cmsg_len = arraysize(cbuf); hdr.msg_control = cmsg; hdr.msg_controllen = arraysize(cbuf); @@ -209,7 +208,7 @@ WriteResult QuicSocketUtils::WritePacket(int fd, } else { hdr.msg_control = cbuf; hdr.msg_controllen = kSpaceForIp; - cmsghdr *cmsg = CMSG_FIRSTHDR(&hdr); + cmsghdr* cmsg = CMSG_FIRSTHDR(&hdr); SetIpInfoInCmsg(self_address, cmsg); hdr.msg_controllen = cmsg->cmsg_len; } diff --git a/net/tools/quic/quic_socket_utils.h b/net/tools/quic/quic_socket_utils.h index 41d3b93c..2c8fe43 100644 --- a/net/tools/quic/quic_socket_utils.h +++ b/net/tools/quic/quic_socket_utils.h @@ -24,12 +24,12 @@ class QuicSocketUtils { // If the msghdr contains IP_PKTINFO or IPV6_PKTINFO, this will return the // IPAddressNumber in that header. Returns an uninitialized IPAddress on // failure. - static IPAddressNumber GetAddressFromMsghdr(struct msghdr *hdr); + static IPAddressNumber GetAddressFromMsghdr(struct msghdr* hdr); // If the msghdr contains an SO_RXQ_OVFL entry, this will set dropped_packets // to the correct value and return true. Otherwise it will return false. - static bool GetOverflowFromMsghdr(struct msghdr *hdr, - QuicPacketCount *dropped_packets); + static bool GetOverflowFromMsghdr(struct msghdr* hdr, + QuicPacketCount* dropped_packets); // Sets either IP_PKTINFO or IPV6_PKTINFO on the socket, based on // address_family. Returns the return code from setsockopt. diff --git a/net/tools/quic/quic_time_wait_list_manager.cc b/net/tools/quic/quic_time_wait_list_manager.cc index f54a465..fba4e87 100644 --- a/net/tools/quic/quic_time_wait_list_manager.cc +++ b/net/tools/quic/quic_time_wait_list_manager.cc @@ -28,8 +28,9 @@ namespace tools { namespace { -// Time period for which the connection_id should live in time wait state. -const int kTimeWaitSeconds = 5; +// Time period for which a given connection_id should live in the time-wait +// state. +int64 FLAGS_quic_time_wait_list_seconds = 5; } // namespace @@ -90,7 +91,8 @@ QuicTimeWaitListManager::QuicTimeWaitListManager( EpollServer* epoll_server, const QuicVersionVector& supported_versions) : epoll_server_(epoll_server), - kTimeWaitPeriod_(QuicTime::Delta::FromSeconds(kTimeWaitSeconds)), + kTimeWaitPeriod_( + QuicTime::Delta::FromSeconds(FLAGS_quic_time_wait_list_seconds)), connection_id_clean_up_alarm_(new ConnectionIdCleanUpAlarm(this)), clock_(epoll_server_), writer_(writer), @@ -282,6 +284,7 @@ void QuicTimeWaitListManager::CleanUpOldConnectionIds() { break; } // This connection_id has lived its age, retire it now. + DVLOG(1) << "Retiring " << it->first << " from the time-wait state."; delete it->second.close_packet; connection_id_map_.erase(it); } diff --git a/net/tools/quic/test_tools/quic_test_utils.h b/net/tools/quic/test_tools/quic_test_utils.h index d70b22d..202edf3 100644 --- a/net/tools/quic/test_tools/quic_test_utils.h +++ b/net/tools/quic/test_tools/quic_test_utils.h @@ -29,7 +29,7 @@ namespace tools { namespace test { static const QuicConnectionId kTestConnectionId = 42; -static const int kTestPort = 123; +static const uint16 kTestPort = 123; static const uint32 kInitialStreamFlowControlWindowForTest = 32 * 1024; // 32 KB static const uint32 kInitialSessionFlowControlWindowForTest = |