diff options
author | rtenneti <rtenneti@chromium.org> | 2015-01-05 15:45:36 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-01-05 23:47:14 +0000 |
commit | fbf7d6b0052f55f5eca633b72c5bb2c5c1e04b9a (patch) | |
tree | 82bf0b2e51a86c21e98a33c456c9dc38906fcb66 /net | |
parent | 502b00b39ec4ecc5e9e745fd22a00cc196209008 (diff) | |
download | chromium_src-fbf7d6b0052f55f5eca633b72c5bb2c5c1e04b9a.zip chromium_src-fbf7d6b0052f55f5eca633b72c5bb2c5c1e04b9a.tar.gz chromium_src-fbf7d6b0052f55f5eca633b72c5bb2c5c1e04b9a.tar.bz2 |
Land Recent QUIC Changes.
QUIC client StartConnect and CryptoConnect return void instead of bool.
No behavior change.
CryptoConnect always returns true in QuicCryptoClientStream, and
StartConnect just passes this up the stack. No point checking an always
true value.
Merge internal change: 83014290
https://codereview.chromium.org/811973006/
After internal change: cl/82509991, setting of a sensible client flow
control window is done in QuicClient::Initialize() and so no longer
needs to be set explicitly in each client.
Only touching QUIC clients: example client
Merge internal change: 82959151
https://codereview.chromium.org/830573003/
Minor change to implementation of QUIC Pacing Sender.
No behavior change, not flag protected.
Move the setting of burst_tokens_ from TimeUntilSend to OnPacketSent, so
it no longer needs to be mutable.
Merge internal change: 82655556
https://codereview.chromium.org/833563002/
Delete inaccurate comment from QuicSpdyClientStream
Merge internal change: 82545048
https://codereview.chromium.org/826273002/
QUIC DCHECK becomes LOG_IF(DFATAL) for better error signals.
NextPendingRetransmissions will dereference an invalid pointer if it is
called with an empty list. Right now, this is enforced via a DCHECK,
which amounts to just documentation in opt mode.
Rather than just dying with an invalid pointer reference, I've changed
this to a LOG_IF(DFATAL,..), so that we have a slightly better signal
about where the problem occurred when it occurred, not later in a bad
dereference.
Merge internal change: 82541587
https://codereview.chromium.org/825733003/
Adds varz for tracking QUIC connections on time-wait list.
Third try on this seemingly simple CL.
Unfortunately, the plan to use callbacks from varz into the time-wait
list manager is a no-go. The reasons for NOT using callbacks are:
- Messy memory semantics. We needed a permanent callback, registered
with the global stats object, for stats collection. At the same
time, we had to guarantee that the global stats object never called
the callback after the time-wait list manager was deleted. The
memory-management aspect of the callbacks was very brittle.
- Need for new locks. We'd need to add a locks to prevent
thread-contention between the stats-collection object and the
time-wait list manager. It's much cleaner to leave all the locking
inside the ExportedVariableMap, than introduce a new lock inside the
time-wait list manager or the stats collection object
Reverted back my original design.
There is now a varz that is mapped per-silo, per-port, as Alyssa
suggested. It tracks the length of the time-wait list per dispatcher.
Added QUIC varz tracking per-dispatcher time-wait list length.
Merge internal change: 82527019
https://codereview.chromium.org/803723003/
R=rch@chromium.org
Review URL: https://codereview.chromium.org/832713003
Cr-Commit-Position: refs/heads/master@{#310000}
Diffstat (limited to 'net')
32 files changed, 177 insertions, 140 deletions
diff --git a/net/quic/congestion_control/pacing_sender.cc b/net/quic/congestion_control/pacing_sender.cc index b31d357..62c64ea 100644 --- a/net/quic/congestion_control/pacing_sender.cc +++ b/net/quic/congestion_control/pacing_sender.cc @@ -56,6 +56,10 @@ bool PacingSender::OnPacketSent( if (has_retransmittable_data != HAS_RETRANSMITTABLE_DATA) { return in_flight; } + if (bytes_in_flight == 0) { + // Add more burst tokens anytime the connection is leaving quiescence. + burst_tokens_ = initial_packet_burst_; + } if (burst_tokens_ > 0) { --burst_tokens_; was_last_send_delayed_ = false; @@ -107,12 +111,8 @@ QuicTime::Delta PacingSender::TimeUntilSend( HasRetransmittableData has_retransmittable_data) const { QuicTime::Delta time_until_send = sender_->TimeUntilSend(now, bytes_in_flight, has_retransmittable_data); - if (bytes_in_flight == 0) { - // Add more burst tokens anytime the connection is entering quiescence. - burst_tokens_ = initial_packet_burst_; - } - if (burst_tokens_ > 0) { - // Don't pace if we have burst tokens available. + if (burst_tokens_ > 0 || bytes_in_flight == 0) { + // Don't pace if we have burst tokens available or leaving quiescence. return time_until_send; } diff --git a/net/quic/congestion_control/pacing_sender.h b/net/quic/congestion_control/pacing_sender.h index 2fdbfce..4531dce 100644 --- a/net/quic/congestion_control/pacing_sender.h +++ b/net/quic/congestion_control/pacing_sender.h @@ -2,11 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // -// A send algorithm which adds pacing on top of an another send algorithm. -// It uses the underlying sender's bandwidth estimate to determine the -// pacing rate to be used. It also takes into consideration the expected -// resolution of the underlying alarm mechanism to ensure that alarms are -// not set too aggressively, and to smooth out variations. +// A send algorithm that adds pacing on top of an another send algorithm. +// It uses the underlying sender's pacing rate to schedule packets. +// It also takes into consideration the expected granularity of the underlying +// alarm to ensure that alarms are not set too aggressively, and err towards +// sending packets too early instead of too late. #ifndef NET_QUIC_CONGESTION_CONTROL_PACING_SENDER_H_ #define NET_QUIC_CONGESTION_CONTROL_PACING_SENDER_H_ @@ -70,9 +70,12 @@ class NET_EXPORT_PRIVATE PacingSender : public SendAlgorithmInterface { private: scoped_ptr<SendAlgorithmInterface> sender_; // Underlying sender. - QuicTime::Delta alarm_granularity_; - uint32 initial_packet_burst_; - mutable uint32 burst_tokens_; + // The estimated system alarm granularity. + const QuicTime::Delta alarm_granularity_; + // Configured size of the burst coming out of quiescence. + const uint32 initial_packet_burst_; + // Number of unpaced packets to be sent before packets are delayed. + uint32 burst_tokens_; // Send time of the last packet considered delayed. QuicTime last_delayed_packet_sent_time_; QuicTime ideal_next_packet_send_time_; // When can the next packet be sent. diff --git a/net/quic/congestion_control/pacing_sender_test.cc b/net/quic/congestion_control/pacing_sender_test.cc index 67585ac..3c240e2 100644 --- a/net/quic/congestion_control/pacing_sender_test.cc +++ b/net/quic/congestion_control/pacing_sender_test.cc @@ -45,50 +45,35 @@ class PacingSenderTest : public ::testing::Test { EXPECT_CALL(*mock_sender_, PacingRate()).WillRepeatedly(Return(bandwidth)); } - void CheckPacketIsSentImmediately() { + void CheckPacketIsSentImmediately(HasRetransmittableData retransmittable_data, + QuicByteCount bytes_in_flight) { // In order for the packet to be sendable, the underlying sender must // permit it to be sent immediately. for (int i = 0; i < 2; ++i) { - EXPECT_CALL(*mock_sender_, TimeUntilSend(clock_.Now(), - kBytesInFlight, - HAS_RETRANSMITTABLE_DATA)) + EXPECT_CALL(*mock_sender_, TimeUntilSend(clock_.Now(), bytes_in_flight, + retransmittable_data)) .WillOnce(Return(zero_time_)); // Verify that the packet can be sent immediately. EXPECT_EQ(zero_time_, - pacing_sender_->TimeUntilSend(clock_.Now(), - kBytesInFlight, - HAS_RETRANSMITTABLE_DATA)); + pacing_sender_->TimeUntilSend(clock_.Now(), bytes_in_flight, + retransmittable_data)); } // Actually send the packet. EXPECT_CALL(*mock_sender_, - OnPacketSent(clock_.Now(), kBytesInFlight, sequence_number_, - kMaxPacketSize, HAS_RETRANSMITTABLE_DATA)); - pacing_sender_->OnPacketSent(clock_.Now(), kBytesInFlight, + OnPacketSent(clock_.Now(), bytes_in_flight, sequence_number_, + kMaxPacketSize, retransmittable_data)); + pacing_sender_->OnPacketSent(clock_.Now(), bytes_in_flight, sequence_number_++, kMaxPacketSize, - HAS_RETRANSMITTABLE_DATA); + retransmittable_data); } - void CheckAckIsSentImmediately() { - // In order for the ack to be sendable, the underlying sender must - // permit it to be sent immediately. - EXPECT_CALL(*mock_sender_, TimeUntilSend(clock_.Now(), - 0, - NO_RETRANSMITTABLE_DATA)) - .WillOnce(Return(zero_time_)); - // Verify that the ACK can be sent immediately. - EXPECT_EQ(zero_time_, - pacing_sender_->TimeUntilSend(clock_.Now(), - 0, - NO_RETRANSMITTABLE_DATA)); + void CheckPacketIsSentImmediately() { + CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA, kBytesInFlight); + } - // Actually send the packet. - EXPECT_CALL(*mock_sender_, - OnPacketSent(clock_.Now(), 0, sequence_number_, - kMaxPacketSize, NO_RETRANSMITTABLE_DATA)); - pacing_sender_->OnPacketSent(clock_.Now(), 0, - sequence_number_++, kMaxPacketSize, - NO_RETRANSMITTABLE_DATA); + void CheckAckIsSentImmediately() { + CheckPacketIsSentImmediately(NO_RETRANSMITTABLE_DATA, kBytesInFlight); } void CheckPacketIsDelayed(QuicTime::Delta delay) { @@ -107,6 +92,13 @@ class PacingSenderTest : public ::testing::Test { } } + void UpdateRtt() { + EXPECT_CALL(*mock_sender_, OnCongestionEvent(true, kBytesInFlight, _, _)); + SendAlgorithmInterface::CongestionVector empty_map; + pacing_sender_->OnCongestionEvent(true, kBytesInFlight, empty_map, + empty_map); + } + const QuicTime::Delta zero_time_; const QuicTime::Delta infinite_time_; MockClock clock_; @@ -147,9 +139,7 @@ TEST_F(PacingSenderTest, VariousSending) { kMaxPacketSize, QuicTime::Delta::FromMilliseconds(1))); // Now update the RTT and verify that packets are actually paced. - EXPECT_CALL(*mock_sender_, OnCongestionEvent(true, kBytesInFlight, _, _)); - SendAlgorithmInterface::CongestionVector empty_map; - pacing_sender_->OnCongestionEvent(true, kBytesInFlight, empty_map, empty_map); + UpdateRtt(); CheckPacketIsSentImmediately(); CheckPacketIsSentImmediately(); @@ -212,9 +202,7 @@ TEST_F(PacingSenderTest, DISABLED_CongestionAvoidanceSending) { kMaxPacketSize * 1.25, QuicTime::Delta::FromMilliseconds(2)))); // Now update the RTT and verify that packets are actually paced. - EXPECT_CALL(*mock_sender_, OnCongestionEvent(true, kBytesInFlight, _, _)); - SendAlgorithmInterface::CongestionVector empty_map; - pacing_sender_->OnCongestionEvent(true, kBytesInFlight, empty_map, empty_map); + UpdateRtt(); CheckPacketIsSentImmediately(); CheckPacketIsSentImmediately(); @@ -270,9 +258,7 @@ TEST_F(PacingSenderTest, InitialBurst) { kMaxPacketSize, QuicTime::Delta::FromMilliseconds(1))); // Update the RTT and verify that the first 10 packets aren't paced. - EXPECT_CALL(*mock_sender_, OnCongestionEvent(true, kBytesInFlight, _, _)); - SendAlgorithmInterface::CongestionVector empty_map; - pacing_sender_->OnCongestionEvent(true, kBytesInFlight, empty_map, empty_map); + UpdateRtt(); // Send 10 packets, and verify that they are not paced. for (int i = 0 ; i < kInitialBurstPackets; ++i) { @@ -288,18 +274,10 @@ TEST_F(PacingSenderTest, InitialBurst) { clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(5)); CheckPacketIsSentImmediately(); - // Next time TimeUntilSend is called with no bytes in flight, the tokens - // should be refilled and there should be no delay. - EXPECT_CALL(*mock_sender_, - TimeUntilSend(clock_.Now(), - 0, - HAS_RETRANSMITTABLE_DATA)). - WillOnce(Return(zero_time_)); - EXPECT_EQ(zero_time_, - pacing_sender_->TimeUntilSend(clock_.Now(), - 0, - HAS_RETRANSMITTABLE_DATA)); - for (int i = 0 ; i < kInitialBurstPackets; ++i) { + // Next time TimeUntilSend is called with no bytes in flight, pacing should + // allow a packet to be sent, and when it's sent, the tokens are refilled. + CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA, 0); + for (int i = 0; i < kInitialBurstPackets - 1; ++i) { CheckPacketIsSentImmediately(); } @@ -332,17 +310,9 @@ TEST_F(PacingSenderTest, InitialBurstNoRttMeasurement) { // Next time TimeUntilSend is called with no bytes in flight, the tokens // should be refilled and there should be no delay. - EXPECT_CALL(*mock_sender_, - TimeUntilSend(clock_.Now(), - 0, - HAS_RETRANSMITTABLE_DATA)). - WillOnce(Return(zero_time_)); - EXPECT_EQ(zero_time_, - pacing_sender_->TimeUntilSend(clock_.Now(), - 0, - HAS_RETRANSMITTABLE_DATA)); + CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA, 0); // Send 10 packets, and verify that they are not paced. - for (int i = 0 ; i < kInitialBurstPackets; ++i) { + for (int i = 0; i < kInitialBurstPackets - 1; ++i) { CheckPacketIsSentImmediately(); } @@ -361,9 +331,7 @@ TEST_F(PacingSenderTest, FastSending) { 2 * kMaxPacketSize, QuicTime::Delta::FromMilliseconds(1))); // Update the RTT and verify that the first 10 packets aren't paced. - EXPECT_CALL(*mock_sender_, OnCongestionEvent(true, kBytesInFlight, _, _)); - SendAlgorithmInterface::CongestionVector empty_map; - pacing_sender_->OnCongestionEvent(true, kBytesInFlight, empty_map, empty_map); + UpdateRtt(); // Send 10 packets, and verify that they are not paced. for (int i = 0; i < kInitialBurstPackets; ++i) { @@ -382,12 +350,8 @@ TEST_F(PacingSenderTest, FastSending) { // Next time TimeUntilSend is called with no bytes in flight, the tokens // should be refilled and there should be no delay. - EXPECT_CALL(*mock_sender_, - TimeUntilSend(clock_.Now(), 0, HAS_RETRANSMITTABLE_DATA)) - .WillOnce(Return(zero_time_)); - EXPECT_EQ(zero_time_, pacing_sender_->TimeUntilSend( - clock_.Now(), 0, HAS_RETRANSMITTABLE_DATA)); - for (int i = 0; i < kInitialBurstPackets; ++i) { + CheckPacketIsSentImmediately(HAS_RETRANSMITTABLE_DATA, 0); + for (int i = 0; i < kInitialBurstPackets - 1; ++i) { CheckPacketIsSentImmediately(); } diff --git a/net/quic/quic_client_session.cc b/net/quic/quic_client_session.cc index 1a99cf9..8a41c6a 100644 --- a/net/quic/quic_client_session.cc +++ b/net/quic/quic_client_session.cc @@ -492,11 +492,7 @@ int QuicClientSession::CryptoConnect(bool require_confirmation, handshake_start_ = base::TimeTicks::Now(); RecordHandshakeState(STATE_STARTED); DCHECK(flow_controller()); - if (!crypto_stream_->CryptoConnect()) { - // TODO(wtc): change crypto_stream_.CryptoConnect() to return a - // QuicErrorCode and map it to a net error code. - return ERR_CONNECTION_FAILED; - } + crypto_stream_->CryptoConnect(); if (IsCryptoHandshakeConfirmed()) return OK; diff --git a/net/quic/quic_crypto_client_stream.cc b/net/quic/quic_crypto_client_stream.cc index 6777b2e..ad78d31 100644 --- a/net/quic/quic_crypto_client_stream.cc +++ b/net/quic/quic_crypto_client_stream.cc @@ -122,10 +122,9 @@ void QuicCryptoClientStream::OnHandshakeMessage( DoHandshakeLoop(&message); } -bool QuicCryptoClientStream::CryptoConnect() { +void QuicCryptoClientStream::CryptoConnect() { next_state_ = STATE_INITIALIZE; DoHandshakeLoop(nullptr); - return true; } int QuicCryptoClientStream::num_sent_client_hellos() const { diff --git a/net/quic/quic_crypto_client_stream.h b/net/quic/quic_crypto_client_stream.h index ae9304c..230cb94 100644 --- a/net/quic/quic_crypto_client_stream.h +++ b/net/quic/quic_crypto_client_stream.h @@ -34,10 +34,8 @@ class NET_EXPORT_PRIVATE QuicCryptoClientStream : public QuicCryptoStream { // CryptoFramerVisitorInterface implementation void OnHandshakeMessage(const CryptoHandshakeMessage& message) override; - // Performs a crypto handshake with the server. Returns true if the crypto - // handshake is started successfully. - // TODO(agl): this should probably return void. - virtual bool CryptoConnect(); + // Performs a crypto handshake with the server. + virtual void CryptoConnect(); // num_sent_client_hellos returns the number of client hello messages that // have been sent. If the handshake has completed then this is one greater diff --git a/net/quic/quic_crypto_client_stream_test.cc b/net/quic/quic_crypto_client_stream_test.cc index 584b6fb..efb5214 100644 --- a/net/quic/quic_crypto_client_stream_test.cc +++ b/net/quic/quic_crypto_client_stream_test.cc @@ -41,7 +41,7 @@ class QuicCryptoClientStreamTest : public ::testing::Test { } void CompleteCryptoHandshake() { - EXPECT_TRUE(stream_->CryptoConnect()); + stream_->CryptoConnect(); CryptoTestUtils::HandshakeWithFakeServer(connection_, stream_.get()); } @@ -81,7 +81,7 @@ TEST_F(QuicCryptoClientStreamTest, MessageAfterHandshake) { } TEST_F(QuicCryptoClientStreamTest, BadMessageType) { - EXPECT_TRUE(stream_->CryptoConnect()); + stream_->CryptoConnect(); message_.set_tag(kCHLO); ConstructHandshakeMessage(); @@ -134,9 +134,8 @@ TEST_F(QuicCryptoClientStreamTest, ExpiredServerConfig) { connection_->AdvanceTime( QuicTime::Delta::FromSeconds(60 * 60 * 24 * 365 * 5)); - // Check that a client hello was sent and that CryptoConnect doesn't fail - // with an error. - EXPECT_TRUE(stream_->CryptoConnect()); + stream_->CryptoConnect(); + // Check that a client hello was sent. ASSERT_EQ(1u, connection_->packets_.size()); } diff --git a/net/quic/quic_crypto_server_stream_test.cc b/net/quic/quic_crypto_server_stream_test.cc index ebb5af2..d15ff2d 100644 --- a/net/quic/quic_crypto_server_stream_test.cc +++ b/net/quic/quic_crypto_server_stream_test.cc @@ -151,7 +151,7 @@ TEST_P(QuicCryptoServerStreamTest, ZeroRTT) { // Do a first handshake in order to prime the client config with the server's // information. - CHECK(client->CryptoConnect()); + client->CryptoConnect(); CHECK_EQ(1u, client_conn->packets_.size()); scoped_ptr<TestSession> server_session(new TestSession(server_conn, config_)); @@ -186,7 +186,7 @@ TEST_P(QuicCryptoServerStreamTest, ZeroRTT) { server_session.get())); server_session->SetCryptoStream(server.get()); - CHECK(client->CryptoConnect()); + client->CryptoConnect(); if (AsyncStrikeRegisterVerification()) { EXPECT_FALSE(client->handshake_confirmed()); diff --git a/net/quic/quic_dispatcher.cc b/net/quic/quic_dispatcher.cc index d27b454..6350d63 100644 --- a/net/quic/quic_dispatcher.cc +++ b/net/quic/quic_dispatcher.cc @@ -345,6 +345,16 @@ void QuicDispatcher::OnWriteBlocked( write_blocked_list_.insert(make_pair(blocked_writer, true)); } +void QuicDispatcher::OnConnectionAddedToTimeWaitList( + QuicConnectionId connection_id) { + DVLOG(1) << "Connection " << connection_id << " added to time wait list."; +} + +void QuicDispatcher::OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) { + DVLOG(1) << "Connection " << connection_id << " removed from time wait list."; +} + QuicSession* QuicDispatcher::CreateQuicSession( QuicConnectionId connection_id, const IPEndPoint& server_address, diff --git a/net/quic/quic_dispatcher.h b/net/quic/quic_dispatcher.h index 640cec8..8e2ce03 100644 --- a/net/quic/quic_dispatcher.h +++ b/net/quic/quic_dispatcher.h @@ -108,6 +108,15 @@ class QuicDispatcher : public QuicBlockedWriterInterface, // Queues the blocked writer for later resumption. void OnWriteBlocked(QuicBlockedWriterInterface* blocked_writer) override; + // Called whenever the QuicTimeWaitListManager adds a new connection to the + // time-wait list. + void OnConnectionAddedToTimeWaitList(QuicConnectionId connection_id) override; + + // Called whenever the QuicTimeWaitListManager removes an old connection from + // the time-wait list. + void OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) override; + typedef base::hash_map<QuicConnectionId, QuicSession*> SessionMap; // Deletes all sessions on the closed session list and clears the list. diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc index 8f62d57..f1804f9 100644 --- a/net/quic/quic_sent_packet_manager.cc +++ b/net/quic/quic_sent_packet_manager.cc @@ -418,7 +418,9 @@ bool QuicSentPacketManager::HasPendingRetransmissions() const { QuicSentPacketManager::PendingRetransmission QuicSentPacketManager::NextPendingRetransmission() { - DCHECK(!pending_retransmissions_.empty()); + LOG_IF(DFATAL, pending_retransmissions_.empty()) + << "Unexpected call to PendingRetransmissions() with empty pending " + << "retransmission list. Corrupted memory usage imminent."; QuicPacketSequenceNumber sequence_number = pending_retransmissions_.begin()->first; TransmissionType transmission_type = pending_retransmissions_.begin()->second; diff --git a/net/quic/quic_sent_packet_manager.h b/net/quic/quic_sent_packet_manager.h index ea6b77d..287b5fc 100644 --- a/net/quic/quic_sent_packet_manager.h +++ b/net/quic/quic_sent_packet_manager.h @@ -146,7 +146,8 @@ class NET_EXPORT_PRIVATE QuicSentPacketManager { // Returns true if there are pending retransmissions. bool HasPendingRetransmissions() const; - // Retrieves the next pending retransmission. + // Retrieves the next pending retransmission. You must ensure that + // there are pending retransmissions prior to calling this function. PendingRetransmission NextPendingRetransmission(); bool HasUnackedPackets() const; diff --git a/net/quic/quic_server_session.h b/net/quic/quic_server_session.h index 5614b21..a463eec 100644 --- a/net/quic/quic_server_session.h +++ b/net/quic/quic_server_session.h @@ -33,7 +33,7 @@ class ReliableQuicStream; // An interface from the session to the entity owning the session. // This lets the session notify its owner (the Dispatcher) when the connection -// is closed or blocked. +// is closed, blocked, or added/removed from the time-wait list. class QuicServerSessionVisitor { public: virtual ~QuicServerSessionVisitor() {} @@ -41,6 +41,12 @@ class QuicServerSessionVisitor { virtual void OnConnectionClosed(QuicConnectionId connection_id, QuicErrorCode error) = 0; virtual void OnWriteBlocked(QuicBlockedWriterInterface* blocked_writer) = 0; + // Called after the given connection is added to the time-wait list. + virtual void OnConnectionAddedToTimeWaitList(QuicConnectionId connection_id) { + } + // Called after the given connection is removed from the time-wait list. + virtual void OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) {} }; class QuicServerSession : public QuicSession { diff --git a/net/quic/quic_time_wait_list_manager.cc b/net/quic/quic_time_wait_list_manager.cc index 8f1f5641..984d95f 100644 --- a/net/quic/quic_time_wait_list_manager.cc +++ b/net/quic/quic_time_wait_list_manager.cc @@ -112,10 +112,10 @@ void QuicTimeWaitListManager::AddConnectionIdToTimeWait( QuicConnectionId connection_id, QuicVersion version, QuicEncryptedPacket* close_packet) { - DVLOG(1) << "Adding " << connection_id << " to the time wait list."; int num_packets = 0; ConnectionIdMap::iterator it = connection_id_map_.find(connection_id); - if (it != connection_id_map_.end()) { // Replace record if it is reinserted. + const bool new_connection_id = it == connection_id_map_.end(); + if (!new_connection_id) { // Replace record if it is reinserted. num_packets = it->second.num_packets; delete it->second.close_packet; connection_id_map_.erase(it); @@ -125,6 +125,9 @@ void QuicTimeWaitListManager::AddConnectionIdToTimeWait( helper_->GetClock()->ApproximateNow(), close_packet); connection_id_map_.insert(make_pair(connection_id, data)); + if (new_connection_id) { + visitor_->OnConnectionAddedToTimeWaitList(connection_id); + } } bool QuicTimeWaitListManager::IsConnectionIdInTimeWait( @@ -277,10 +280,11 @@ void QuicTimeWaitListManager::CleanUpOldConnectionIds() { if (now.Subtract(oldest_connection_id) < kTimeWaitPeriod_) { break; } + const QuicConnectionId connection_id = it->first; // 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); + visitor_->OnConnectionRemovedFromTimeWaitList(connection_id); } SetConnectionIdCleanUpAlarm(); } diff --git a/net/quic/quic_time_wait_list_manager.h b/net/quic/quic_time_wait_list_manager.h index a01ad2e..d6be4c0 100644 --- a/net/quic/quic_time_wait_list_manager.h +++ b/net/quic/quic_time_wait_list_manager.h @@ -89,6 +89,9 @@ class QuicTimeWaitListManager : public QuicBlockedWriterInterface { // QuicVersion associated with it. QuicVersion GetQuicVersionFromConnectionId(QuicConnectionId connection_id); + // The number of connections on the time-wait list. + size_t num_connections() const { return connection_id_map_.size(); } + protected: virtual QuicEncryptedPacket* BuildPublicReset( const QuicPublicResetPacket& packet); diff --git a/net/quic/test_tools/crypto_test_utils.cc b/net/quic/test_tools/crypto_test_utils.cc index b24316f..5661676 100644 --- a/net/quic/test_tools/crypto_test_utils.cc +++ b/net/quic/test_tools/crypto_test_utils.cc @@ -243,7 +243,7 @@ int CryptoTestUtils::HandshakeWithFakeClient( &crypto_config); client_session.SetCryptoStream(&client); - CHECK(client.CryptoConnect()); + client.CryptoConnect(); CHECK_EQ(1u, client_conn->packets_.size()); CommunicateHandshakeMessagesAndRunCallbacks( diff --git a/net/quic/test_tools/mock_crypto_client_stream.cc b/net/quic/test_tools/mock_crypto_client_stream.cc index b45d371..8650eb9 100644 --- a/net/quic/test_tools/mock_crypto_client_stream.cc +++ b/net/quic/test_tools/mock_crypto_client_stream.cc @@ -34,7 +34,7 @@ void MockCryptoClientStream::OnHandshakeMessage( CloseConnection(QUIC_CRYPTO_MESSAGE_AFTER_HANDSHAKE_COMPLETE); } -bool MockCryptoClientStream::CryptoConnect() { +void MockCryptoClientStream::CryptoConnect() { switch (handshake_mode_) { case ZERO_RTT: { encryption_established_ = true; @@ -67,7 +67,6 @@ bool MockCryptoClientStream::CryptoConnect() { break; } } - return true; } void MockCryptoClientStream::SendOnCryptoHandshakeEvent( diff --git a/net/quic/test_tools/mock_crypto_client_stream.h b/net/quic/test_tools/mock_crypto_client_stream.h index 636314a..d0bda38 100644 --- a/net/quic/test_tools/mock_crypto_client_stream.h +++ b/net/quic/test_tools/mock_crypto_client_stream.h @@ -48,7 +48,7 @@ class MockCryptoClientStream : public QuicCryptoClientStream { void OnHandshakeMessage(const CryptoHandshakeMessage& message) override; // QuicCryptoClientStream implementation. - bool CryptoConnect() override; + void CryptoConnect() override; // Invokes the sessions's CryptoHandshakeEvent method with the specified // event. diff --git a/net/tools/quic/quic_client.cc b/net/tools/quic/quic_client.cc index 056f858..d7d35c8 100644 --- a/net/tools/quic/quic_client.cc +++ b/net/tools/quic/quic_client.cc @@ -190,16 +190,14 @@ bool QuicClient::CreateUDPSocket() { } bool QuicClient::Connect() { - if (!StartConnect()) { - return false; - } + StartConnect(); while (EncryptionBeingEstablished()) { WaitForEvents(); } return session_->connection()->connected(); } -bool QuicClient::StartConnect() { +void QuicClient::StartConnect() { DCHECK(initialized_); DCHECK(!connected()); @@ -224,7 +222,7 @@ bool QuicClient::StartConnect() { writer_.reset(writer); } session_->InitializeSession(server_id_, &crypto_config_); - return session_->CryptoConnect(); + session_->CryptoConnect(); } bool QuicClient::EncryptionBeingEstablished() { diff --git a/net/tools/quic/quic_client.h b/net/tools/quic/quic_client.h index c095a4d..47d4cec 100644 --- a/net/tools/quic/quic_client.h +++ b/net/tools/quic/quic_client.h @@ -75,7 +75,7 @@ class QuicClient : public EpollCallbackInterface, // Start the crypto handshake. This can be done in place of the synchronous // Connect(), but callers are responsible for making sure the crypto handshake // completes. - bool StartConnect(); + void StartConnect(); // Returns true if the crypto handshake has yet to establish encryption. // Returns false if encryption is active (even if the server hasn't confirmed diff --git a/net/tools/quic/quic_client_bin.cc b/net/tools/quic/quic_client_bin.cc index 0f5c746..6fcde96 100644 --- a/net/tools/quic/quic_client_bin.cc +++ b/net/tools/quic/quic_client_bin.cc @@ -117,22 +117,12 @@ int main(int argc, char *argv[]) { << " with supported versions " << QuicVersionVectorToString(versions); net::EpollServer epoll_server; - net::QuicConfig config; - - // The default flow control window of 16 Kb is too small for practical - // purposes. Set it to the specified value, which has a large default. - config.SetInitialFlowControlWindowToSend( - FLAGS_flow_control_window_bytes); - config.SetInitialStreamFlowControlWindowToSend( - FLAGS_flow_control_window_bytes); - config.SetInitialSessionFlowControlWindowToSend( - FLAGS_flow_control_window_bytes); net::tools::QuicClient client( net::IPEndPoint(addr, FLAGS_port), net::QuicServerId(FLAGS_hostname, FLAGS_port, FLAGS_secure, net::PRIVACY_MODE_DISABLED), - versions, true, config, &epoll_server); + versions, true, &epoll_server); client.Initialize(); diff --git a/net/tools/quic/quic_client_session.cc b/net/tools/quic/quic_client_session.cc index 235eeb0..815fdfd 100644 --- a/net/tools/quic/quic_client_session.cc +++ b/net/tools/quic/quic_client_session.cc @@ -61,9 +61,9 @@ QuicCryptoClientStream* QuicClientSession::GetCryptoStream() { return crypto_stream_.get(); } -bool QuicClientSession::CryptoConnect() { +void QuicClientSession::CryptoConnect() { DCHECK(flow_controller()); - return crypto_stream_->CryptoConnect(); + crypto_stream_->CryptoConnect(); } int QuicClientSession::GetNumSentClientHellos() const { diff --git a/net/tools/quic/quic_client_session.h b/net/tools/quic/quic_client_session.h index 3c92efa..7532552 100644 --- a/net/tools/quic/quic_client_session.h +++ b/net/tools/quic/quic_client_session.h @@ -40,9 +40,8 @@ class QuicClientSession : public QuicClientSessionBase { void OnProofVerifyDetailsAvailable( const ProofVerifyDetails& verify_details) override; - // Performs a crypto handshake with the server. Returns true if the crypto - // handshake is started successfully. - bool CryptoConnect(); + // Performs a crypto handshake with the server. + void CryptoConnect(); // Returns the number of client hello messages that have been sent on the // crypto stream. If the handshake has completed then this is one greater diff --git a/net/tools/quic/quic_client_session_test.cc b/net/tools/quic/quic_client_session_test.cc index 496211f..1e8006b 100644 --- a/net/tools/quic/quic_client_session_test.cc +++ b/net/tools/quic/quic_client_session_test.cc @@ -51,7 +51,7 @@ class ToolsQuicClientSessionTest } void CompleteCryptoHandshake() { - ASSERT_TRUE(session_->CryptoConnect()); + session_->CryptoConnect(); CryptoTestUtils::HandshakeWithFakeServer( connection_, session_->GetCryptoStream()); } diff --git a/net/tools/quic/quic_dispatcher.cc b/net/tools/quic/quic_dispatcher.cc index 4ea48e9..fa32fc0 100644 --- a/net/tools/quic/quic_dispatcher.cc +++ b/net/tools/quic/quic_dispatcher.cc @@ -353,6 +353,16 @@ void QuicDispatcher::OnWriteBlocked( write_blocked_list_.insert(make_pair(blocked_writer, true)); } +void QuicDispatcher::OnConnectionAddedToTimeWaitList( + QuicConnectionId connection_id) { + DVLOG(1) << "Connection " << connection_id << " added to time wait list."; +} + +void QuicDispatcher::OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) { + DVLOG(1) << "Connection " << connection_id << " removed from time wait list."; +} + QuicPacketWriter* QuicDispatcher::CreateWriter(int fd) { return new QuicDefaultPacketWriter(fd); } diff --git a/net/tools/quic/quic_dispatcher.h b/net/tools/quic/quic_dispatcher.h index fc557c4..509c5e5 100644 --- a/net/tools/quic/quic_dispatcher.h +++ b/net/tools/quic/quic_dispatcher.h @@ -110,6 +110,15 @@ class QuicDispatcher : public QuicServerSessionVisitor, // Queues the blocked writer for later resumption. void OnWriteBlocked(QuicBlockedWriterInterface* blocked_writer) override; + // Called whenever the QuicTimeWaitListManager adds a new connection + // to the time-wait list. + void OnConnectionAddedToTimeWaitList(QuicConnectionId connection_id) override; + + // Called whenever the QuicTimeWaitListManager removes an old connection + // from the time-wait list. + void OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) override; + typedef base::hash_map<QuicConnectionId, QuicSession*> SessionMap; // Deletes all sessions on the closed session list and clears the list. diff --git a/net/tools/quic/quic_server_session.h b/net/tools/quic/quic_server_session.h index 81cae12..3b5cff0 100644 --- a/net/tools/quic/quic_server_session.h +++ b/net/tools/quic/quic_server_session.h @@ -34,7 +34,7 @@ class QuicServerSessionPeer; // An interface from the session to the entity owning the session. // This lets the session notify its owner (the Dispatcher) when the connection -// is closed or blocked. +// is closed, blocked, or added/removed from the time-wait list. class QuicServerSessionVisitor { public: virtual ~QuicServerSessionVisitor() {} @@ -42,6 +42,12 @@ class QuicServerSessionVisitor { virtual void OnConnectionClosed(QuicConnectionId connection_id, QuicErrorCode error) = 0; virtual void OnWriteBlocked(QuicBlockedWriterInterface* blocked_writer) = 0; + // Called after the given connection is added to the time-wait list. + virtual void OnConnectionAddedToTimeWaitList(QuicConnectionId connection_id) { + } + // Called after the given connection is removed from the time-wait list. + virtual void OnConnectionRemovedFromTimeWaitList( + QuicConnectionId connection_id) {} }; class QuicServerSession : public QuicSession { diff --git a/net/tools/quic/quic_spdy_client_stream.cc b/net/tools/quic/quic_spdy_client_stream.cc index 0237b89..3618df6 100644 --- a/net/tools/quic/quic_spdy_client_stream.cc +++ b/net/tools/quic/quic_spdy_client_stream.cc @@ -118,7 +118,6 @@ int QuicSpdyClientStream::ParseResponseHeaders() { return len; } -// Sends body data to the server and returns the number of bytes sent. void QuicSpdyClientStream::SendBody(const string& data, bool fin) { WriteOrBufferData(data, fin, nullptr); } diff --git a/net/tools/quic/quic_time_wait_list_manager.cc b/net/tools/quic/quic_time_wait_list_manager.cc index fba4e87..ed6addc 100644 --- a/net/tools/quic/quic_time_wait_list_manager.cc +++ b/net/tools/quic/quic_time_wait_list_manager.cc @@ -114,10 +114,10 @@ void QuicTimeWaitListManager::AddConnectionIdToTimeWait( QuicConnectionId connection_id, QuicVersion version, QuicEncryptedPacket* close_packet) { - DVLOG(1) << "Adding " << connection_id << " to the time wait list."; int num_packets = 0; ConnectionIdMap::iterator it = connection_id_map_.find(connection_id); - if (it != connection_id_map_.end()) { // Replace record if it is reinserted. + const bool new_connection_id = it == connection_id_map_.end(); + if (!new_connection_id) { // Replace record if it is reinserted. num_packets = it->second.num_packets; delete it->second.close_packet; connection_id_map_.erase(it); @@ -127,6 +127,9 @@ void QuicTimeWaitListManager::AddConnectionIdToTimeWait( clock_.ApproximateNow(), close_packet); connection_id_map_.insert(make_pair(connection_id, data)); + if (new_connection_id) { + visitor_->OnConnectionAddedToTimeWaitList(connection_id); + } } bool QuicTimeWaitListManager::IsConnectionIdInTimeWait( @@ -283,10 +286,11 @@ void QuicTimeWaitListManager::CleanUpOldConnectionIds() { if (now.Subtract(oldest_connection_id) < kTimeWaitPeriod_) { break; } + const QuicConnectionId connection_id = it->first; // 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); + visitor_->OnConnectionRemovedFromTimeWaitList(connection_id); } SetConnectionIdCleanUpAlarm(); } diff --git a/net/tools/quic/quic_time_wait_list_manager.h b/net/tools/quic/quic_time_wait_list_manager.h index cf049ed1..a00a646 100644 --- a/net/tools/quic/quic_time_wait_list_manager.h +++ b/net/tools/quic/quic_time_wait_list_manager.h @@ -93,6 +93,9 @@ class QuicTimeWaitListManager : public QuicBlockedWriterInterface { // QuicVersion associated with it. QuicVersion GetQuicVersionFromConnectionId(QuicConnectionId connection_id); + // The number of connections on the time-wait list. + size_t num_connections() const { return connection_id_map_.size(); } + protected: virtual QuicEncryptedPacket* BuildPublicReset( const QuicPublicResetPacket& packet); diff --git a/net/tools/quic/quic_time_wait_list_manager_test.cc b/net/tools/quic/quic_time_wait_list_manager_test.cc index 4e5d7c3..17c26d8 100644 --- a/net/tools/quic/quic_time_wait_list_manager_test.cc +++ b/net/tools/quic/quic_time_wait_list_manager_test.cc @@ -216,12 +216,14 @@ Matcher<const std::tr1::tuple<const char*, int> > PublicResetPacketEq( TEST_F(QuicTimeWaitListManagerTest, CheckConnectionIdInTimeWait) { EXPECT_FALSE(IsConnectionIdInTimeWait(connection_id_)); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_)); AddConnectionId(connection_id_); EXPECT_TRUE(IsConnectionIdInTimeWait(connection_id_)); } TEST_F(QuicTimeWaitListManagerTest, SendConnectionClose) { size_t kConnectionCloseLength = 100; + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_)); AddConnectionId( connection_id_, QuicVersionMax(), @@ -237,6 +239,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendConnectionClose) { } TEST_F(QuicTimeWaitListManagerTest, SendPublicReset) { + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_)); AddConnectionId(connection_id_); const int kRandomSequenceNumber = 1; EXPECT_CALL(writer_, WritePacket(_, _, @@ -250,6 +253,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendPublicReset) { } TEST_F(QuicTimeWaitListManagerTest, SendPublicResetWithExponentialBackOff) { + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_)); AddConnectionId(connection_id_); for (int sequence_number = 1; sequence_number < 101; ++sequence_number) { if ((sequence_number & (sequence_number - 1)) == 0) { @@ -277,6 +281,7 @@ TEST_F(QuicTimeWaitListManagerTest, CleanUpOldConnectionIds) { for (int connection_id = 1; connection_id <= kOldConnectionIdCount; ++connection_id) { + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id)); AddConnectionId(connection_id); } @@ -288,6 +293,7 @@ TEST_F(QuicTimeWaitListManagerTest, CleanUpOldConnectionIds) { for (int connection_id = kOldConnectionIdCount + 1; connection_id <= kConnectionIdCount; ++connection_id) { + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id)); AddConnectionId(connection_id); } @@ -300,6 +306,12 @@ TEST_F(QuicTimeWaitListManagerTest, CleanUpOldConnectionIds) { time_wait_period.Subtract(offset).ToMicroseconds(); EXPECT_CALL(epoll_server_, RegisterAlarm(next_alarm_time, _)); + for (int connection_id = 1; connection_id <= kConnectionIdCount; + ++connection_id) { + if (connection_id <= kOldConnectionIdCount) { + EXPECT_CALL(visitor_, OnConnectionRemovedFromTimeWaitList(connection_id)); + } + } time_wait_list_manager_.CleanUpOldConnectionIds(); for (int connection_id = 1; connection_id <= kConnectionIdCount; @@ -313,6 +325,7 @@ TEST_F(QuicTimeWaitListManagerTest, CleanUpOldConnectionIds) { TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { QuicConnectionId connection_id = 1; + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id)); AddConnectionId(connection_id); QuicPacketSequenceNumber sequence_number = 234; scoped_ptr<QuicEncryptedPacket> packet(ConstructEncryptedPacket( @@ -343,6 +356,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { // write packet should not be called since we are write blocked but the // should be queued. QuicConnectionId other_connection_id = 2; + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(other_connection_id)); AddConnectionId(other_connection_id); QuicPacketSequenceNumber other_sequence_number = 23423; scoped_ptr<QuicEncryptedPacket> other_packet( @@ -376,6 +390,9 @@ TEST_F(QuicTimeWaitListManagerTest, GetQuicVersionFromMap) { const int kConnectionId2 = 456; const int kConnectionId3 = 789; + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(kConnectionId1)); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(kConnectionId2)); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(kConnectionId3)); AddConnectionId(kConnectionId1, QuicVersionMin(), nullptr); AddConnectionId(kConnectionId2, QuicVersionMax(), nullptr); AddConnectionId(kConnectionId3, QuicVersionMax(), nullptr); @@ -394,6 +411,7 @@ TEST_F(QuicTimeWaitListManagerTest, GetQuicVersionFromMap) { TEST_F(QuicTimeWaitListManagerTest, AddConnectionIdTwice) { // Add connection_ids such that their expiry time is kTimeWaitPeriod_. epoll_server_.set_now_in_usec(0); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(connection_id_)); AddConnectionId(connection_id_); EXPECT_TRUE(IsConnectionIdInTimeWait(connection_id_)); size_t kConnectionCloseLength = 100; @@ -424,6 +442,7 @@ TEST_F(QuicTimeWaitListManagerTest, AddConnectionIdTwice) { time_wait_period.ToMicroseconds(); EXPECT_CALL(epoll_server_, RegisterAlarm(next_alarm_time, _)); + EXPECT_CALL(visitor_, OnConnectionRemovedFromTimeWaitList(connection_id_)); time_wait_list_manager_.CleanUpOldConnectionIds(); EXPECT_FALSE(IsConnectionIdInTimeWait(connection_id_)); } @@ -440,8 +459,10 @@ TEST_F(QuicTimeWaitListManagerTest, ConnectionIdsOrderedByTime) { // 1 will hash lower than 2, but we add it later. They should come out in the // add order, not hash order. epoll_server_.set_now_in_usec(0); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(kConnectionId1)); AddConnectionId(kConnectionId1); epoll_server_.set_now_in_usec(10); + EXPECT_CALL(visitor_, OnConnectionAddedToTimeWaitList(kConnectionId2)); AddConnectionId(kConnectionId2); const QuicTime::Delta time_wait_period = @@ -450,6 +471,7 @@ TEST_F(QuicTimeWaitListManagerTest, ConnectionIdsOrderedByTime) { EXPECT_CALL(epoll_server_, RegisterAlarm(_, _)); + EXPECT_CALL(visitor_, OnConnectionRemovedFromTimeWaitList(kConnectionId1)); time_wait_list_manager_.CleanUpOldConnectionIds(); EXPECT_FALSE(IsConnectionIdInTimeWait(kConnectionId1)); EXPECT_TRUE(IsConnectionIdInTimeWait(kConnectionId2)); diff --git a/net/tools/quic/test_tools/quic_test_utils.h b/net/tools/quic/test_tools/quic_test_utils.h index 202edf3..394041d 100644 --- a/net/tools/quic/test_tools/quic_test_utils.h +++ b/net/tools/quic/test_tools/quic_test_utils.h @@ -157,6 +157,10 @@ class MockQuicServerSessionVisitor : public QuicServerSessionVisitor { QuicErrorCode error)); MOCK_METHOD1(OnWriteBlocked, void(QuicBlockedWriterInterface* blocked_writer)); + MOCK_METHOD1(OnConnectionAddedToTimeWaitList, + void(QuicConnectionId connection_id)); + MOCK_METHOD1(OnConnectionRemovedFromTimeWaitList, + void(QuicConnectionId connection_id)); private: DISALLOW_COPY_AND_ASSIGN(MockQuicServerSessionVisitor); |