diff options
author | rtenneti@chromium.org <rtenneti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-01 18:42:39 +0000 |
---|---|---|
committer | rtenneti@chromium.org <rtenneti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-08-01 18:42:39 +0000 |
commit | 66cd2d6525e7f75460d16c7fa8f8e4e2eae85ef4 (patch) | |
tree | 65dc9276b02807790f9a67a61b6e98af3c76a927 /net/quic | |
parent | 2a0262ff309d82c61d1c2d53c103091cdebbe0c7 (diff) | |
download | chromium_src-66cd2d6525e7f75460d16c7fa8f8e4e2eae85ef4.zip chromium_src-66cd2d6525e7f75460d16c7fa8f8e4e2eae85ef4.tar.gz chromium_src-66cd2d6525e7f75460d16c7fa8f8e4e2eae85ef4.tar.bz2 |
Land Recent QUIC Changes
Fix to QUIC's RttStats ExpireSmoothedMetrics where the mean deviation
may increase if the rtt decreased.
Merge internal change: 71665066
https://codereview.chromium.org/406403002/
Allow client to request Reno congestion control.
Disabled BBR tests (will enable after BBR code is checked in).
Merge internal change: 71621257
https://codereview.chromium.org/412753002/
QUIC - Minor cleanup change to keep internal source tree and chromium
codebase in sync.
+ Use DECHEK_EQ instead of DCHECK.
Merge internal change: 71613624
https://codereview.chromium.org/412733002/
Allow QuicConnection to own its QuicPacketWriter
Added a new "bool owns_writer" parameter to the QuicConnection
constructor that specifies whether the QuicConnection has ownership.
Also cleaned up the ownership semantics in a number of tests --
previously, the writer was not actually outliving the connection.
Merge internal change: 71590341
https://codereview.chromium.org/413573003/
Added QuicTime::Infinite() to create infinite time required by the
following internal change.
Change QUIC"s BBR sender to only fall out of slow start when there is a
packet loss if the residual is less than 0.25.
Merge internal change: 71436337
https://codereview.chromium.org/410743003/
Ensure the UnackedPacketMap only keeps one previous transmission less
than largest_observed.
Also improves the tightness of QuicUnackedPacketMapTest.
Merge internal change: 71426216
https://codereview.chromium.org/411723003/
Parameterize the QUIC end-to-end tests to run with FEC both enabled and
disabled.
Found bug that was fixed in internal change: 71390633
(https://codereview.chromium.org/414523003/)
Merge internal change: 71393479
https://codereview.chromium.org/412543005/
Fix a bug in the QuicReceivedPacketManager where a packet which had
previously been revied was not removed from the revived packet set when
it was eventually received out of order.
Merge internal change: 71390633
https://codereview.chromium.org/414523003/
QuicConnection::ProcessValidatedPacket doesn't need to be public because
it is only called by QuicConnection::OnPacketHeader.
Merge internal change: 71371063
https://codereview.chromium.org/402333003/
Add bandwidth_estimate and server_namespace fields to the source-address
token proto. Not used anywhere, this CL is really just for discussion
about which fields (and fieldnames) we should use.
Merge internal change: 71360060
https://codereview.chromium.org/404343003/
R=rch@chromium.org
Review URL: https://codereview.chromium.org/411823002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287052 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net/quic')
29 files changed, 342 insertions, 60 deletions
diff --git a/net/quic/congestion_control/rtt_stats.cc b/net/quic/congestion_control/rtt_stats.cc index e70d2d9..b03686d 100644 --- a/net/quic/congestion_control/rtt_stats.cc +++ b/net/quic/congestion_control/rtt_stats.cc @@ -4,6 +4,8 @@ #include "net/quic/congestion_control/rtt_stats.h" +#include <complex> // std::abs + using std::max; namespace net { @@ -40,7 +42,10 @@ void RttStats::SampleNewRecentMinRtt(uint32 num_samples) { } void RttStats::ExpireSmoothedMetrics() { - mean_deviation_ = max(mean_deviation_, latest_rtt_.Subtract(smoothed_rtt_)); + mean_deviation_ = + max(mean_deviation_, + QuicTime::Delta::FromMicroseconds( + std::abs(smoothed_rtt_.Subtract(latest_rtt_).ToMicroseconds()))); smoothed_rtt_ = max(smoothed_rtt_, latest_rtt_); } diff --git a/net/quic/congestion_control/rtt_stats.h b/net/quic/congestion_control/rtt_stats.h index 8ceaf21..5e01687 100644 --- a/net/quic/congestion_control/rtt_stats.h +++ b/net/quic/congestion_control/rtt_stats.h @@ -32,8 +32,9 @@ class NET_EXPORT_PRIVATE RttStats { QuicTime::Delta ack_delay, QuicTime now); - // Causes the smoothed_rtt to be increased to the latest_rtt and the - // mean_variance to be increased to the most recent variance. + // Causes the smoothed_rtt to be increased to the latest_rtt if the latest_rtt + // is larger. The mean deviation is increased to the most recent deviation if + // it's larger. void ExpireSmoothedMetrics(); // Forces RttStats to sample a new recent min rtt within the next diff --git a/net/quic/congestion_control/rtt_stats_test.cc b/net/quic/congestion_control/rtt_stats_test.cc index a24016c..6b5ff7d 100644 --- a/net/quic/congestion_control/rtt_stats_test.cc +++ b/net/quic/congestion_control/rtt_stats_test.cc @@ -172,6 +172,13 @@ TEST_F(RttStatsTest, ExpireSmoothedMetrics) { rtt_stats_.ExpireSmoothedMetrics(); EXPECT_EQ(doubled_rtt, rtt_stats_.SmoothedRtt()); EXPECT_EQ(initial_rtt.Multiply(0.875), rtt_stats_.mean_deviation()); + + // Now go back down to 5ms and expire the smoothed metrics, and ensure the + // mean deviation increases to 15ms. + QuicTime::Delta half_rtt = initial_rtt.Multiply(0.5); + rtt_stats_.UpdateRtt(half_rtt, QuicTime::Delta::Zero(), QuicTime::Zero()); + EXPECT_GT(doubled_rtt, rtt_stats_.SmoothedRtt()); + EXPECT_LT(initial_rtt, rtt_stats_.mean_deviation()); } } // namespace test diff --git a/net/quic/congestion_control/send_algorithm_interface.cc b/net/quic/congestion_control/send_algorithm_interface.cc index 2401667..5d0e032 100644 --- a/net/quic/congestion_control/send_algorithm_interface.cc +++ b/net/quic/congestion_control/send_algorithm_interface.cc @@ -10,8 +10,6 @@ namespace net { -const bool kUseReno = false; - class RttStats; // Factory for send side congestion control algorithm. @@ -21,12 +19,14 @@ SendAlgorithmInterface* SendAlgorithmInterface::Create( CongestionControlType congestion_control_type, QuicConnectionStats* stats) { switch (congestion_control_type) { - case kTCP: - return new TcpCubicSender(clock, rtt_stats, kUseReno, + case kCubic: + return new TcpCubicSender(clock, rtt_stats, + false /* don't use Reno */, + kMaxTcpCongestionWindow, stats); + case kReno: + return new TcpCubicSender(clock, rtt_stats, + true /* use Reno */, kMaxTcpCongestionWindow, stats); - case kInterArrival: - LOG(DFATAL) << "InterArrivalSendAlgorithm no longer supported."; - return NULL; case kFixRateCongestionControl: return new FixRateSender(rtt_stats); case kBBR: diff --git a/net/quic/congestion_control/send_algorithm_simulator.cc b/net/quic/congestion_control/send_algorithm_simulator.cc index 5c55f5b..36aa321 100644 --- a/net/quic/congestion_control/send_algorithm_simulator.cc +++ b/net/quic/congestion_control/send_algorithm_simulator.cc @@ -73,7 +73,9 @@ void SendAlgorithmSimulator::TransferBytes() { void SendAlgorithmSimulator::TransferBytes(QuicByteCount max_bytes, QuicTime::Delta max_time) { - const QuicTime end_time = clock_->Now().Add(max_time); + const QuicTime end_time = max_time.IsInfinite() ? + QuicTime::Zero().Add(QuicTime::Delta::Infinite()) : + clock_->Now().Add(max_time); QuicByteCount bytes_sent = 0; while (!pending_transfers_.empty() && clock_->Now() < end_time && diff --git a/net/quic/crypto/crypto_protocol.h b/net/quic/crypto/crypto_protocol.h index 9e38d4b..5779137 100644 --- a/net/quic/crypto/crypto_protocol.h +++ b/net/quic/crypto/crypto_protocol.h @@ -51,6 +51,7 @@ const QuicTag kINAR = TAG('I', 'N', 'A', 'R'); // Inter arrival // Congestion control options const QuicTag kTBBR = TAG('T', 'B', 'B', 'R'); // Reduced Buffer Bloat TCP +const QuicTag kRENO = TAG('R', 'E', 'N', 'O'); // Reno Congestion Control // Loss detection algorithm types const QuicTag kNACK = TAG('N', 'A', 'C', 'K'); // TCP style nack counting diff --git a/net/quic/crypto/quic_crypto_server_config.cc b/net/quic/crypto/quic_crypto_server_config.cc index 3659270..ec88594c 100644 --- a/net/quic/crypto/quic_crypto_server_config.cc +++ b/net/quic/crypto/quic_crypto_server_config.cc @@ -466,7 +466,7 @@ bool QuicCryptoServerConfig::SetConfigs( configs_.swap(new_configs); SelectNewPrimaryConfig(now); - DCHECK(primary_config_); + DCHECK(primary_config_.get()); DCHECK_EQ(configs_.find(primary_config_->id)->second, primary_config_); } @@ -507,8 +507,8 @@ void QuicCryptoServerConfig::ValidateClientHello( if (!next_config_promotion_time_.IsZero() && next_config_promotion_time_.IsAfter(now)) { SelectNewPrimaryConfig(now); - DCHECK(primary_config_); - DCHECK(configs_.find(primary_config_->id)->second == primary_config_); + DCHECK(primary_config_.get()); + DCHECK_EQ(configs_.find(primary_config_->id)->second, primary_config_); } memcpy(primary_orbit, primary_config_->orbit, sizeof(primary_orbit)); @@ -580,8 +580,8 @@ QuicErrorCode QuicCryptoServerConfig::ProcessClientHello( if (!next_config_promotion_time_.IsZero() && next_config_promotion_time_.IsAfter(now)) { SelectNewPrimaryConfig(now); - DCHECK(primary_config_); - DCHECK(configs_.find(primary_config_->id)->second == primary_config_); + DCHECK(primary_config_.get()); + DCHECK_EQ(configs_.find(primary_config_->id)->second, primary_config_); } // We'll use the config that the client requested in order to do diff --git a/net/quic/crypto/source_address_token.cc b/net/quic/crypto/source_address_token.cc index b095e76..f20c343 100644 --- a/net/quic/crypto/source_address_token.cc +++ b/net/quic/crypto/source_address_token.cc @@ -14,6 +14,12 @@ using std::vector; namespace net { +CachedNetworkParameters::CachedNetworkParameters() { +} + +CachedNetworkParameters::~CachedNetworkParameters() { +} + SourceAddressToken::SourceAddressToken() { } @@ -27,6 +33,8 @@ string SourceAddressToken::SerializeAsString() const { string time_str = base::Int64ToString(timestamp_); out.push_back(time_str.size()); out.append(time_str); + // TODO(rtenneti): Implement serialization of optional CachedNetworkParameters + // when they are used. return out; } @@ -52,6 +60,9 @@ bool SourceAddressToken::ParseFromArray(const char* plaintext, ip_.assign(&plaintext[1], ip_len); timestamp_ = timestamp; + + // TODO(rtenneti): Implement parsing of optional CachedNetworkParameters when + // they are used. return true; } diff --git a/net/quic/crypto/source_address_token.h b/net/quic/crypto/source_address_token.h index fbb50b1..c719fef 100644 --- a/net/quic/crypto/source_address_token.h +++ b/net/quic/crypto/source_address_token.h @@ -13,6 +13,69 @@ namespace net { // TODO(rtenneti): sync with server more rationally. +// CachedNetworkParameters contains data that can be used to choose appropriate +// connection parameters (initial RTT, initial CWND, etc.) in new connections. +class CachedNetworkParameters { + public: + // Describes the state of the connection during which the supplied network + // parameters were calculated. + enum PreviousConnectionState { + SLOW_START = 0, + CONGESTION_AVOIDANCE = 1, + }; + + CachedNetworkParameters(); + ~CachedNetworkParameters(); + + std::string serving_region() const { + return serving_region_; + } + void set_serving_region(base::StringPiece serving_region) { + serving_region_ = serving_region.as_string(); + } + + int32 bandwidth_estimate_bytes_per_second() const { + return bandwidth_estimate_bytes_per_second_; + } + void set_bandwidth_estimate_bytes_per_second( + int32 bandwidth_estimate_bytes_per_second) { + bandwidth_estimate_bytes_per_second_ = bandwidth_estimate_bytes_per_second; + } + + int32 min_rtt_ms() const { + return min_rtt_ms_; + } + void set_min_rtt_ms(int32 min_rtt_ms) { + min_rtt_ms_ = min_rtt_ms; + } + + int32 previous_connection_state() const { + return previous_connection_state_; + } + void set_previous_connection_state(int32 previous_connection_state) { + previous_connection_state_ = previous_connection_state; + } + + private: + // serving_region_ is used to decide whether or not the bandwidth estimate and + // min RTT are reasonable and if they should be used. + // For example a group of geographically close servers may share the same + // serving_region_ string if they are expected to have similar network + // performance. + std::string serving_region_; + // The server can supply a bandwidth estimate (in bytes/s) which it may re-use + // on receipt of a source-address token with this field set. + int32 bandwidth_estimate_bytes_per_second_; + // The min RTT seen on a previous connection can be used by the server to + // inform initial connection parameters for new connections. + int32 min_rtt_ms_; + // Encodes the PreviousConnectionState enum. + int32 previous_connection_state_; +}; + +// TODO(rtenneti): sync with server more rationally. +// A SourceAddressToken is serialised, encrypted and sent to clients so that +// they can prove ownership of an IP address. class SourceAddressToken { public: SourceAddressToken(); @@ -25,23 +88,37 @@ class SourceAddressToken { std::string ip() const { return ip_; } - - int64 timestamp() const { - return timestamp_; - } - void set_ip(base::StringPiece ip) { ip_ = ip.as_string(); } + int64 timestamp() const { + return timestamp_; + } void set_timestamp(int64 timestamp) { timestamp_ = timestamp; } + const CachedNetworkParameters& cached_network_parameters() const { + return cached_network_parameters_; + } + void set_cached_network_parameters( + const CachedNetworkParameters& cached_network_parameters) { + cached_network_parameters_ = cached_network_parameters; + } + private: + // ip_ contains either 4 (IPv4) or 16 (IPv6) bytes of IP address in network + // byte order. std::string ip_; + // timestamp_ contains a UNIX timestamp value of the time when the token was + // created. int64 timestamp_; + // The server can provide estimated network parameters to be used for + // initial parameter selection in future connections. + CachedNetworkParameters cached_network_parameters_; + DISALLOW_COPY_AND_ASSIGN(SourceAddressToken); }; diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc index 7d89917..2a091ca 100644 --- a/net/quic/quic_connection.cc +++ b/net/quic/quic_connection.cc @@ -191,12 +191,14 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id, IPEndPoint address, QuicConnectionHelperInterface* helper, QuicPacketWriter* writer, + bool owns_writer, bool is_server, const QuicVersionVector& supported_versions) : framer_(supported_versions, helper->GetClock()->ApproximateNow(), is_server), helper_(helper), writer_(writer), + owns_writer_(owns_writer), encryption_level_(ENCRYPTION_NONE), clock_(helper->GetClock()), random_generator_(helper->GetRandomGenerator()), @@ -250,6 +252,9 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id, } QuicConnection::~QuicConnection() { + if (owns_writer_) { + delete writer_; + } STLDeleteElements(&undecryptable_packets_); STLDeleteValues(&group_map_); for (QueuedPacketList::iterator it = queued_packets_.begin(); diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h index 8c6d2ca..a7e8a7e 100644 --- a/net/quic/quic_connection.h +++ b/net/quic/quic_connection.h @@ -223,11 +223,13 @@ class NET_EXPORT_PRIVATE QuicConnection }; // Constructs a new QuicConnection for |connection_id| and |address|. - // |helper| and |writer| must outlive this connection. + // |helper| must outlive this connection, and if |owns_writer| is false, so + // must |writer|. QuicConnection(QuicConnectionId connection_id, IPEndPoint address, QuicConnectionHelperInterface* helper, QuicPacketWriter* writer, + bool owns_writer, bool is_server, const QuicVersionVector& supported_versions); virtual ~QuicConnection(); @@ -303,11 +305,6 @@ class NET_EXPORT_PRIVATE QuicConnection // If the socket is not blocked, writes queued packets. void WriteIfNotBlocked(); - // Do any work which logically would be done in OnPacket but can not be - // safely done until the packet is validated. Returns true if the packet - // can be handled, false otherwise. - virtual bool ProcessValidatedPacket(); - // The version of the protocol this connection is using. QuicVersion version() const { return framer_.version(); } @@ -490,6 +487,11 @@ class NET_EXPORT_PRIVATE QuicConnection }; protected: + // Do any work which logically would be done in OnPacket but can not be + // safely done until the packet is validated. Returns true if the packet + // can be handled, false otherwise. + virtual bool ProcessValidatedPacket(); + // Send a packet to the peer using encryption |level|. If |sequence_number| // is present in the |retransmission_map_|, then contents of this packet will // be retransmitted with a new sequence number if it's not acked by the peer. @@ -632,7 +634,8 @@ class NET_EXPORT_PRIVATE QuicConnection QuicFramer framer_; QuicConnectionHelperInterface* helper_; // Not owned. - QuicPacketWriter* writer_; // Not owned. + QuicPacketWriter* writer_; // Owned or not depending on |owns_writer_|. + bool owns_writer_; EncryptionLevel encryption_level_; const QuicClock* clock_; QuicRandom* random_generator_; diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc index a0587b5..b868c98 100644 --- a/net/quic/quic_connection_test.cc +++ b/net/quic/quic_connection_test.cc @@ -415,7 +415,12 @@ class TestConnection : public QuicConnection { TestPacketWriter* writer, bool is_server, QuicVersion version) - : QuicConnection(connection_id, address, helper, writer, is_server, + : QuicConnection(connection_id, + address, + helper, + writer, + false /* owns_writer */, + is_server, SupportedVersions(version)), writer_(writer) { // Disable tail loss probes for most tests. diff --git a/net/quic/quic_dispatcher.cc b/net/quic/quic_dispatcher.cc index ae964ed..5c6836a 100644 --- a/net/quic/quic_dispatcher.cc +++ b/net/quic/quic_dispatcher.cc @@ -377,13 +377,23 @@ QuicConnection* QuicDispatcher::CreateQuicConnection( QuicConnection* connection; if (FLAGS_enable_quic_connection_flow_control_2) { DVLOG(1) << "Creating QuicDispatcher with all versions."; - connection = new QuicConnection(connection_id, client_address, helper_, - writer, true, supported_versions_); + connection = new QuicConnection(connection_id, + client_address, + helper_, + writer, + false /* owns_writer */, + true /* is_server */, + supported_versions_); } else { DVLOG(1) << "Connection flow control disabled, creating QuicDispatcher " << "WITHOUT version 19 or higher."; connection = new QuicConnection( - connection_id, client_address, helper_, writer, true, + connection_id, + client_address, + helper_, + writer, + false /* owns_writer */, + true /* is_server */, supported_versions_no_connection_flow_control_); } writer->set_connection(connection); diff --git a/net/quic/quic_http_stream_test.cc b/net/quic/quic_http_stream_test.cc index 2b85417..5e46258 100644 --- a/net/quic/quic_http_stream_test.cc +++ b/net/quic/quic_http_stream_test.cc @@ -59,7 +59,12 @@ class TestQuicConnection : public QuicConnection { IPEndPoint address, QuicConnectionHelper* helper, QuicPacketWriter* writer) - : QuicConnection(connection_id, address, helper, writer, false, + : QuicConnection(connection_id, + address, + helper, + writer, + false /* owns_writer */, + false /* is_server */, versions) { } diff --git a/net/quic/quic_received_packet_manager.cc b/net/quic/quic_received_packet_manager.cc index 3b47687..1a1a1d3 100644 --- a/net/quic/quic_received_packet_manager.cc +++ b/net/quic/quic_received_packet_manager.cc @@ -178,6 +178,8 @@ void QuicReceivedPacketManager::RecordPacketReceived( receive_algorithm_->RecordIncomingPacket( bytes, sequence_number, receipt_time); + + received_info_.revived_packets.erase(sequence_number); } void QuicReceivedPacketManager::RecordPacketRevived( diff --git a/net/quic/quic_received_packet_manager_test.cc b/net/quic/quic_received_packet_manager_test.cc index b579309..b2a52c8 100644 --- a/net/quic/quic_received_packet_manager_test.cc +++ b/net/quic/quic_received_packet_manager_test.cc @@ -202,6 +202,10 @@ class QuicReceivedPacketManagerTest : public ::testing::Test { received_manager_.RecordPacketReceived(0u, header, receipt_time); } + void RecordPacketRevived(QuicPacketSequenceNumber sequence_number) { + received_manager_.RecordPacketRevived(sequence_number); + } + QuicConnectionStats stats_; QuicReceivedPacketManager received_manager_; }; @@ -328,6 +332,32 @@ TEST_F(QuicReceivedPacketManagerTest, UpdateReceivedConnectionStats) { EXPECT_EQ(1u, stats_.packets_reordered); } +TEST_F(QuicReceivedPacketManagerTest, RevivedPacket) { + RecordPacketReceipt(1, 0); + RecordPacketReceipt(3, 0); + RecordPacketRevived(2); + + ReceivedPacketInfo info; + received_manager_.UpdateReceivedPacketInfo(&info, QuicTime::Zero()); + EXPECT_EQ(1u, info.missing_packets.size()); + EXPECT_EQ(2u, *info.missing_packets.begin()); + EXPECT_EQ(1u, info.revived_packets.size()); + EXPECT_EQ(2u, *info.missing_packets.begin()); +} + +TEST_F(QuicReceivedPacketManagerTest, PacketRevivedThenReceived) { + RecordPacketReceipt(1, 0); + RecordPacketReceipt(3, 0); + RecordPacketRevived(2); + RecordPacketReceipt(2, 0); + + ReceivedPacketInfo info; + received_manager_.UpdateReceivedPacketInfo(&info, QuicTime::Zero()); + EXPECT_TRUE(info.missing_packets.empty()); + EXPECT_TRUE(info.revived_packets.empty()); +} + + } // namespace } // namespace test } // namespace net diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc index 30f34e3..c51b744 100644 --- a/net/quic/quic_sent_packet_manager.cc +++ b/net/quic/quic_sent_packet_manager.cc @@ -109,6 +109,11 @@ void QuicSentPacketManager::SetFromConfig(const QuicConfig& config) { send_algorithm_.reset( SendAlgorithmInterface::Create(clock_, &rtt_stats_, kBBR, stats_)); } + if (config.HasReceivedConnectionOptions() && + ContainsQuicTag(config.ReceivedConnectionOptions(), kRENO)) { + send_algorithm_.reset( + SendAlgorithmInterface::Create(clock_, &rtt_stats_, kReno, stats_)); + } if (config.congestion_feedback() == kPACE || (config.HasReceivedConnectionOptions() && ContainsQuicTag(config.ReceivedConnectionOptions(), kPACE))) { diff --git a/net/quic/quic_sent_packet_manager_test.cc b/net/quic/quic_sent_packet_manager_test.cc index 90f94f7..99ed479 100644 --- a/net/quic/quic_sent_packet_manager_test.cc +++ b/net/quic/quic_sent_packet_manager_test.cc @@ -1392,6 +1392,29 @@ TEST_F(QuicSentPacketManagerTest, NegotiateTimeLossDetectionFromOptions) { &manager_)->GetLossDetectionType()); } +TEST_F(QuicSentPacketManagerTest, NegotiateCongestionControlFromOptions) { + QuicConfig config; + QuicTagVector options; + + options.push_back(kRENO); + QuicConfigPeer::SetReceivedConnectionOptions(&config, options); + EXPECT_CALL(*network_change_visitor_, OnCongestionWindowChange(_)); + manager_.SetFromConfig(config); + EXPECT_EQ(kReno, QuicSentPacketManagerPeer::GetCongestionControlAlgorithm( + manager_)->GetCongestionControlType()); + + // TODO(rtenneti): Enable the following code after BBR code is checked in. +#if 0 + options.clear(); + options.push_back(kTBBR); + QuicConfigPeer::SetReceivedConnectionOptions(&config, options); + EXPECT_CALL(*network_change_visitor_, OnCongestionWindowChange(_)); + manager_.SetFromConfig(config); + EXPECT_EQ(kBBR, QuicSentPacketManagerPeer::GetCongestionControlAlgorithm( + manager_)->GetCongestionControlType()); +#endif +} + TEST_F(QuicSentPacketManagerTest, NegotiatePacingFromOptions) { ValueRestore<bool> old_flag(&FLAGS_enable_quic_pacing, true); EXPECT_FALSE(manager_.using_pacing()); diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc index c399456..503e549 100644 --- a/net/quic/quic_stream_factory.cc +++ b/net/quic/quic_stream_factory.cc @@ -833,9 +833,13 @@ int QuicStreamFactory::CreateSession( clock_.get(), random_generator_)); } - QuicConnection* connection = - new QuicConnection(connection_id, addr, helper_.get(), writer.get(), - false, supported_versions_); + QuicConnection* connection = new QuicConnection(connection_id, + addr, + helper_.get(), + writer.get(), + false /* owns_writer */, + false /* is_server */, + supported_versions_); writer->SetConnection(connection); connection->set_max_packet_length(max_packet_length_); diff --git a/net/quic/quic_time.cc b/net/quic/quic_time.cc index d467980..e20a338 100644 --- a/net/quic/quic_time.cc +++ b/net/quic/quic_time.cc @@ -90,6 +90,11 @@ QuicTime QuicTime::Zero() { } // static +QuicTime QuicTime::Infinite() { + return QuicTime(base::TimeTicks::FromInternalValue(kQuicInfiniteTimeUs)); +} + +// static QuicTime QuicTime::Max(QuicTime time1, QuicTime time2) { return time1 > time2 ? time1 : time2; } diff --git a/net/quic/quic_time.h b/net/quic/quic_time.h index 20cc308..5fbe30c 100644 --- a/net/quic/quic_time.h +++ b/net/quic/quic_time.h @@ -81,6 +81,9 @@ class NET_EXPORT_PRIVATE QuicTime { // will return false for these times. static QuicTime Zero(); + // Creates a new QuicTime with an infinite time. + static QuicTime Infinite(); + // Returns the later time of time1 and time2. static QuicTime Max(QuicTime time1, QuicTime time2); diff --git a/net/quic/quic_unacked_packet_map.cc b/net/quic/quic_unacked_packet_map.cc index 563b53d..a07de8b 100644 --- a/net/quic/quic_unacked_packet_map.cc +++ b/net/quic/quic_unacked_packet_map.cc @@ -74,6 +74,15 @@ void QuicUnackedPacketMap::OnRetransmittedPacket( // We keep the old packet in the unacked packet list until it, or one of // the retransmissions of it are acked. transmission_info->retransmittable_frames = NULL; + // Only keep one transmission older than largest observed, because only the + // most recent is expected to possibly be a spurious retransmission. + if (transmission_info->all_transmissions->size() > 1 && + *(++transmission_info->all_transmissions->begin()) < largest_observed_) { + QuicPacketSequenceNumber old_transmission = + *transmission_info->all_transmissions->begin(); + transmission_info->all_transmissions->erase(old_transmission); + unacked_packets_.erase(old_transmission); + } unacked_packets_[new_sequence_number] = TransmissionInfo(frames, new_sequence_number, diff --git a/net/quic/quic_unacked_packet_map_test.cc b/net/quic/quic_unacked_packet_map_test.cc index c5264fd1..87c6ca8 100644 --- a/net/quic/quic_unacked_packet_map_test.cc +++ b/net/quic/quic_unacked_packet_map_test.cc @@ -33,8 +33,8 @@ class QuicUnackedPacketMapTest : public ::testing::Test { sequence_number, PACKET_1BYTE_SEQUENCE_NUMBER, NULL, 0, NULL); } - void VerifyPendingPackets(QuicPacketSequenceNumber* packets, - size_t num_packets) { + void VerifyInFlightPackets(QuicPacketSequenceNumber* packets, + size_t num_packets) { if (num_packets == 0) { EXPECT_FALSE(unacked_packets_.HasInFlightPackets()); EXPECT_FALSE(unacked_packets_.HasMultipleInFlightPackets()); @@ -43,11 +43,21 @@ class QuicUnackedPacketMapTest : public ::testing::Test { if (num_packets == 1) { EXPECT_TRUE(unacked_packets_.HasInFlightPackets()); EXPECT_FALSE(unacked_packets_.HasMultipleInFlightPackets()); + ASSERT_TRUE(unacked_packets_.IsUnacked(packets[0])); + EXPECT_TRUE(unacked_packets_.GetTransmissionInfo(packets[0]).in_flight); } for (size_t i = 0; i < num_packets; ++i) { ASSERT_TRUE(unacked_packets_.IsUnacked(packets[i])); EXPECT_TRUE(unacked_packets_.GetTransmissionInfo(packets[i]).in_flight); } + size_t in_flight_count = 0; + for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin(); + it != unacked_packets_.end(); ++it) { + if (it->second.in_flight) { + ++in_flight_count; + } + } + EXPECT_EQ(num_packets, in_flight_count); } void VerifyUnackedPackets(QuicPacketSequenceNumber* packets, @@ -61,6 +71,12 @@ class QuicUnackedPacketMapTest : public ::testing::Test { for (size_t i = 0; i < num_packets; ++i) { EXPECT_TRUE(unacked_packets_.IsUnacked(packets[i])) << packets[i]; } + size_t unacked_count = 0; + for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin(); + it != unacked_packets_.end(); ++it) { + ++unacked_count; + } + EXPECT_EQ(num_packets, unacked_count); } void VerifyRetransmittablePackets(QuicPacketSequenceNumber* packets, @@ -90,12 +106,12 @@ TEST_F(QuicUnackedPacketMapTest, RttOnly) { QuicPacketSequenceNumber unacked[] = { 1 }; VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(NULL, 0); + VerifyInFlightPackets(NULL, 0); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.IncreaseLargestObserved(1); VerifyUnackedPackets(NULL, 0); - VerifyPendingPackets(NULL, 0); + VerifyInFlightPackets(NULL, 0); VerifyRetransmittablePackets(NULL, 0); } @@ -106,22 +122,22 @@ TEST_F(QuicUnackedPacketMapTest, RetransmittableInflightAndRtt) { QuicPacketSequenceNumber unacked[] = { 1 }; VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); VerifyRetransmittablePackets(unacked, arraysize(unacked)); unacked_packets_.RemoveRetransmittability(1); VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.IncreaseLargestObserved(1); VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.RemoveFromInFlight(1); VerifyUnackedPackets(NULL, 0); - VerifyPendingPackets(NULL, 0); + VerifyInFlightPackets(NULL, 0); VerifyRetransmittablePackets(NULL, 0); } @@ -135,32 +151,78 @@ TEST_F(QuicUnackedPacketMapTest, RetransmittedPacket) { QuicPacketSequenceNumber unacked[] = { 1, 2 }; VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); QuicPacketSequenceNumber retransmittable[] = { 2 }; VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); unacked_packets_.RemoveRetransmittability(1); VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.IncreaseLargestObserved(2); VerifyUnackedPackets(unacked, arraysize(unacked)); - VerifyPendingPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.RemoveFromInFlight(2); QuicPacketSequenceNumber unacked2[] = { 1 }; VerifyUnackedPackets(unacked, arraysize(unacked2)); - VerifyPendingPackets(unacked, arraysize(unacked2)); + VerifyInFlightPackets(unacked, arraysize(unacked2)); VerifyRetransmittablePackets(NULL, 0); unacked_packets_.RemoveFromInFlight(1); VerifyUnackedPackets(NULL, 0); - VerifyPendingPackets(NULL, 0); + VerifyInFlightPackets(NULL, 0); VerifyRetransmittablePackets(NULL, 0); } +TEST_F(QuicUnackedPacketMapTest, RetransmitTwice) { + // Simulate a retransmittable packet being sent and retransmitted twice. + unacked_packets_.AddPacket(CreateRetransmittablePacket(1)); + unacked_packets_.SetSent(1, now_, kDefaultLength, true); + unacked_packets_.AddPacket(CreateRetransmittablePacket(2)); + unacked_packets_.SetSent(2, now_, kDefaultLength, true); + + QuicPacketSequenceNumber unacked[] = { 1, 2 }; + VerifyUnackedPackets(unacked, arraysize(unacked)); + VerifyInFlightPackets(unacked, arraysize(unacked)); + QuicPacketSequenceNumber retransmittable[] = { 1, 2 }; + VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); + + // Early retransmit 1 as 3 and send new data as 4. + unacked_packets_.IncreaseLargestObserved(2); + unacked_packets_.RemoveFromInFlight(2); + unacked_packets_.RemoveRetransmittability(2); + unacked_packets_.RemoveFromInFlight(1); + unacked_packets_.OnRetransmittedPacket(1, 3, LOSS_RETRANSMISSION); + unacked_packets_.SetSent(3, now_, kDefaultLength, true); + unacked_packets_.AddPacket(CreateRetransmittablePacket(4)); + unacked_packets_.SetSent(4, now_, kDefaultLength, true); + + QuicPacketSequenceNumber unacked2[] = { 1, 3, 4 }; + VerifyUnackedPackets(unacked2, arraysize(unacked2)); + QuicPacketSequenceNumber pending2[] = { 3, 4, }; + VerifyInFlightPackets(pending2, arraysize(pending2)); + QuicPacketSequenceNumber retransmittable2[] = { 3, 4 }; + VerifyRetransmittablePackets(retransmittable2, arraysize(retransmittable2)); + + // Early retransmit 3 (formerly 1) as 5, and remove 1 from unacked. + unacked_packets_.IncreaseLargestObserved(4); + unacked_packets_.RemoveFromInFlight(4); + unacked_packets_.RemoveRetransmittability(4); + unacked_packets_.RemoveFromInFlight(3); + unacked_packets_.OnRetransmittedPacket(3, 5, LOSS_RETRANSMISSION); + unacked_packets_.SetSent(5, now_, kDefaultLength, true); + + QuicPacketSequenceNumber unacked3[] = { 3, 5 }; + VerifyUnackedPackets(unacked3, arraysize(unacked3)); + QuicPacketSequenceNumber pending3[] = { 5 }; + VerifyInFlightPackets(pending3, arraysize(pending3)); + QuicPacketSequenceNumber retransmittable3[] = { 5 }; + VerifyRetransmittablePackets(retransmittable3, arraysize(retransmittable3)); +} + } // namespace } // namespace test } // namespace net diff --git a/net/quic/test_tools/quic_connection_peer.cc b/net/quic/test_tools/quic_connection_peer.cc index e51fb52..c4c63c7 100644 --- a/net/quic/test_tools/quic_connection_peer.cc +++ b/net/quic/test_tools/quic_connection_peer.cc @@ -208,8 +208,13 @@ QuicPacketWriter* QuicConnectionPeer::GetWriter(QuicConnection* connection) { // static void QuicConnectionPeer::SetWriter(QuicConnection* connection, - QuicPacketWriter* writer) { + QuicPacketWriter* writer, + bool owns_writer) { + if (connection->owns_writer_) { + delete connection->writer_; + } connection->writer_ = writer; + connection->owns_writer_ = owns_writer; } // static diff --git a/net/quic/test_tools/quic_connection_peer.h b/net/quic/test_tools/quic_connection_peer.h index 0cc6fbb..11d4814 100644 --- a/net/quic/test_tools/quic_connection_peer.h +++ b/net/quic/test_tools/quic_connection_peer.h @@ -106,7 +106,10 @@ class QuicConnectionPeer { static QuicAlarm* GetTimeoutAlarm(QuicConnection* connection); static QuicPacketWriter* GetWriter(QuicConnection* connection); - static void SetWriter(QuicConnection* connection, QuicPacketWriter* writer); + // If |owns_writer| is true, takes ownership of |writer|. + static void SetWriter(QuicConnection* connection, + QuicPacketWriter* writer, + bool owns_writer); static void CloseConnection(QuicConnection* connection); static QuicEncryptedPacket* GetConnectionClosePacket( QuicConnection* connection); diff --git a/net/quic/test_tools/quic_sent_packet_manager_peer.cc b/net/quic/test_tools/quic_sent_packet_manager_peer.cc index 2b2fcb9..83a08e0 100644 --- a/net/quic/test_tools/quic_sent_packet_manager_peer.cc +++ b/net/quic/test_tools/quic_sent_packet_manager_peer.cc @@ -34,9 +34,9 @@ const LossDetectionInterface* QuicSentPacketManagerPeer::GetLossAlgorithm( // static const SendAlgorithmInterface* -QuicSentPacketManagerPeer::GetCongestionControlAlgorithm( - QuicSentPacketManager* sent_packet_manager) { - return sent_packet_manager->send_algorithm_.get(); + QuicSentPacketManagerPeer::GetCongestionControlAlgorithm( + const QuicSentPacketManager& sent_packet_manager) { + return sent_packet_manager.send_algorithm_.get(); } // static diff --git a/net/quic/test_tools/quic_sent_packet_manager_peer.h b/net/quic/test_tools/quic_sent_packet_manager_peer.h index 77fba0b..18eb6be 100644 --- a/net/quic/test_tools/quic_sent_packet_manager_peer.h +++ b/net/quic/test_tools/quic_sent_packet_manager_peer.h @@ -26,7 +26,7 @@ class QuicSentPacketManagerPeer { QuicSentPacketManager* sent_packet_manager); static const SendAlgorithmInterface* GetCongestionControlAlgorithm( - QuicSentPacketManager* sent_packet_manager); + const QuicSentPacketManager& sent_packet_manager); static void SetLossAlgorithm(QuicSentPacketManager* sent_packet_manager, LossDetectionInterface* loss_detector); diff --git a/net/quic/test_tools/quic_test_utils.cc b/net/quic/test_tools/quic_test_utils.cc index 62a891b..9b54f94 100644 --- a/net/quic/test_tools/quic_test_utils.cc +++ b/net/quic/test_tools/quic_test_utils.cc @@ -230,8 +230,8 @@ MockConnection::MockConnection(bool is_server) IPEndPoint(TestPeerIPAddress(), kTestPort), new testing::NiceMock<MockHelper>(), new testing::NiceMock<MockPacketWriter>(), + true /* owns_writer */, is_server, QuicSupportedVersions()), - writer_(QuicConnectionPeer::GetWriter(this)), helper_(helper()) { } @@ -240,8 +240,8 @@ MockConnection::MockConnection(IPEndPoint address, : QuicConnection(kTestConnectionId, address, new testing::NiceMock<MockHelper>(), new testing::NiceMock<MockPacketWriter>(), + true /* owns_writer */, is_server, QuicSupportedVersions()), - writer_(QuicConnectionPeer::GetWriter(this)), helper_(helper()) { } @@ -251,8 +251,8 @@ MockConnection::MockConnection(QuicConnectionId connection_id, IPEndPoint(TestPeerIPAddress(), kTestPort), new testing::NiceMock<MockHelper>(), new testing::NiceMock<MockPacketWriter>(), + true /* owns_writer */, is_server, QuicSupportedVersions()), - writer_(QuicConnectionPeer::GetWriter(this)), helper_(helper()) { } @@ -262,8 +262,8 @@ MockConnection::MockConnection(bool is_server, IPEndPoint(TestPeerIPAddress(), kTestPort), new testing::NiceMock<MockHelper>(), new testing::NiceMock<MockPacketWriter>(), + true /* owns_writer */, is_server, supported_versions), - writer_(QuicConnectionPeer::GetWriter(this)), helper_(helper()) { } diff --git a/net/quic/test_tools/quic_test_utils.h b/net/quic/test_tools/quic_test_utils.h index 631d59f..3bae37f 100644 --- a/net/quic/test_tools/quic_test_utils.h +++ b/net/quic/test_tools/quic_test_utils.h @@ -315,7 +315,6 @@ class MockConnection : public QuicConnection { } private: - scoped_ptr<QuicPacketWriter> writer_; scoped_ptr<QuicConnectionHelperInterface> helper_; DISALLOW_COPY_AND_ASSIGN(MockConnection); |