From c5e1aca90ddc180362e0663648c482b874436adf Mon Sep 17 00:00:00 2001 From: "rtenneti@chromium.org" Date: Thu, 30 Jan 2014 04:03:04 +0000 Subject: Land Recent QUIC Changes. Cleanup: remove BlockedWriterInterface from QuicPacketWriter. No behavior changes. Merge internal change: 60499316 https://codereview.chromium.org/135363006/ Properly trigger OnCanWrite processing from PacketDroppingTestWriter. Re-enable the LargePostWithPacketLossAndBlockedSocket test. Testing only. Dialing back the loss from 30% to 10% seems to have stabilized the test. Merge internal change: 60479729 https://codereview.chromium.org/131503016/ Use LOG_IF instead of LOG_IF_FIRST_N in a few places, as per avd's suggestion. Merge internal change: 60382807 https://codereview.chromium.org/149163004/ Change the default RTT from 60ms to 100ms, which is more typical of internal server's. Merge internal change: 60366147 https://codereview.chromium.org/146583006/ Remove an impossible check in QuicConnection for when a packet is retransmitted before it is sent. Updating the tests also found and fixed an edge case where a truncated ack could cause the SentPacketManager to raise the high water mark above a pending packet. Merge internal change: 60169343 https://codereview.chromium.org/145123003/ Export primary insecure and secure QUIC config id via internal server status pages. Merge internal change: 60108488 https://codereview.chromium.org/137423015/ Export SCIDs, QUIC secret seed names and orbits to varz. Merge internal change: 60107113 https://codereview.chromium.org/144033006/ Refactor QuicConnection to use explicit notification for getting onto the write blocked list. Remove the PacketWriter interface from QuicDispatcher. Merge internal change: 60103466 Fix build. Build got broken by a race between presubmit tests and me editing the file that was in the process of being submitted. :( Merge internal change: 60104577 https://codereview.chromium.org/149263002/ Remove the word Payload from AddFrame methods in QUIC framer. Merge internal change: 60101552 https://codereview.chromium.org/131513022/ Fixes QUIC's Cubic sender to use correct alpha when in Reno mode. Merge internal change: 60088487 https://codereview.chromium.org/136453013/ Add DFATALs to QuicFramer branches where packet creation fails. Merge internal change: 60077767 https://codereview.chromium.org/148073002/ Fix a QUIC bug where previously undecryptable packets were not decrypted before sending out an ack when the encryption level changed. Merge internal change: 60051502 Added logging for all frame types. Added code to dump frames and packet_headers. Changes to make QuicHttpStreamTest work. https://codereview.chromium.org/144063012/ R=rch@chromium.org Review URL: https://codereview.chromium.org/146033003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@247827 0039d316-1c4b-4281-b951-d872f2087c98 --- net/quic/congestion_control/cubic.cc | 46 ++++-- net/quic/congestion_control/cubic_test.cc | 35 ++-- net/quic/congestion_control/tcp_cubic_sender.cc | 2 +- .../congestion_control/tcp_cubic_sender_test.cc | 4 +- net/quic/crypto/quic_crypto_server_config.cc | 25 +++ net/quic/crypto/quic_crypto_server_config.h | 16 ++ net/quic/quic_client_session_test.cc | 3 +- net/quic/quic_connection.cc | 153 ++++++++--------- net/quic/quic_connection.h | 50 +++--- net/quic/quic_connection_test.cc | 62 +++---- net/quic/quic_default_packet_writer.cc | 3 +- net/quic/quic_default_packet_writer.h | 5 +- net/quic/quic_framer.cc | 38 +++-- net/quic/quic_framer.h | 26 ++- net/quic/quic_framer_test.cc | 7 +- net/quic/quic_http_stream_test.cc | 11 +- net/quic/quic_packet_creator.cc | 1 + net/quic/quic_packet_writer.h | 4 +- net/quic/quic_protocol.cc | 85 +++++++++- net/quic/quic_protocol.h | 15 ++ net/quic/quic_sent_packet_manager.cc | 39 ++--- net/quic/quic_sent_packet_manager.h | 3 - net/quic/quic_sent_packet_manager_test.cc | 183 ++++----------------- net/quic/test_tools/quic_test_packet_maker.cc | 41 +++++ net/quic/test_tools/quic_test_packet_maker.h | 8 + net/quic/test_tools/quic_test_utils.cc | 4 +- net/quic/test_tools/quic_test_utils.h | 5 +- net/tools/quic/end_to_end_test.cc | 53 ++++-- net/tools/quic/quic_client.cc | 1 + net/tools/quic/quic_default_packet_writer.cc | 3 +- net/tools/quic/quic_default_packet_writer.h | 4 +- net/tools/quic/quic_dispatcher.cc | 63 +++---- net/tools/quic/quic_dispatcher.h | 27 +-- net/tools/quic/quic_dispatcher_test.cc | 5 +- net/tools/quic/quic_packet_writer_wrapper.cc | 6 +- net/tools/quic/quic_packet_writer_wrapper.h | 3 +- net/tools/quic/quic_time_wait_list_manager.cc | 3 +- net/tools/quic/quic_time_wait_list_manager_test.cc | 25 +-- .../quic/test_tools/packet_dropping_test_writer.cc | 23 +-- .../quic/test_tools/packet_dropping_test_writer.h | 20 ++- net/tools/quic/test_tools/quic_test_utils.h | 5 +- 41 files changed, 596 insertions(+), 519 deletions(-) (limited to 'net') diff --git a/net/quic/congestion_control/cubic.cc b/net/quic/congestion_control/cubic.cc index 2212098..938faaea 100644 --- a/net/quic/congestion_control/cubic.cc +++ b/net/quic/congestion_control/cubic.cc @@ -26,10 +26,26 @@ const int kCubeScale = 40; // 1024*1024^3 (first 1024 is from 0.100^3) const int kCubeCongestionWindowScale = 410; const uint64 kCubeFactor = (GG_UINT64_C(1) << kCubeScale) / kCubeCongestionWindowScale; -const uint32 kBetaSPDY = 939; // Back off factor after loss for SPDY, reduces - // the CWND by 1/12th. -const uint32 kBetaLastMax = 871; // Additional back off factor after loss for - // the stored max value. + +const uint32 kNumConnections = 2; +const float kBeta = static_cast(0.7); // Default Cubic backoff factor. +// Additional backoff factor when loss occurs in the concave part of the Cubic +// curve. This additional backoff factor is expected to give up bandwidth to +// new concurrent flows and speed up convergence. +const float kBetaLastMax = static_cast(0.85); + +// kNConnectionBeta is the backoff factor after loss for our N-connection +// emulation, which emulates the effective backoff of an ensemble of N TCP-Reno +// connections on a single loss event. The effective multiplier is computed as: +const float kNConnectionBeta = (kNumConnections - 1 + kBeta) / kNumConnections; + +// TCPFriendly alpha is described in Section 3.3 of the CUBIC paper. Note that +// kBeta here is a cwnd multiplier, and is equal to 1-beta from the CUBIC paper. +// We derive the equivalent kNConnectionAlpha for an N-connection emulation as: +const float kNConnectionAlpha = 3 * kNumConnections * kNumConnections * + (1 - kNConnectionBeta) / (1 + kNConnectionBeta); +// TODO(jri): Compute kNConnectionBeta and kNConnectionAlpha from +// number of active streams. } // namespace Cubic::Cubic(const QuicClock* clock) @@ -57,12 +73,12 @@ QuicTcpCongestionWindow Cubic::CongestionWindowAfterPacketLoss( // We never reached the old max, so assume we are competing with another // flow. Use our extra back off factor to allow the other flow to go up. last_max_congestion_window_ = - (kBetaLastMax * current_congestion_window) >> 10; + static_cast(kBetaLastMax * current_congestion_window); } else { last_max_congestion_window_ = current_congestion_window; } epoch_ = QuicTime::Zero(); // Reset time. - return (current_congestion_window * kBetaSPDY) >> 10; + return static_cast(current_congestion_window * kNConnectionBeta); } QuicTcpCongestionWindow Cubic::CongestionWindowAfterAck( @@ -114,13 +130,21 @@ QuicTcpCongestionWindow Cubic::CongestionWindowAfterAck( // We have a new cubic congestion window. last_target_congestion_window_ = target_congestion_window; - // Update estimated TCP congestion_window. - // Note: we do a normal Reno congestion avoidance calculation not the - // calculation described in section 3.3 TCP-friendly region of the document. - while (acked_packets_count_ >= estimated_tcp_congestion_window_) { - acked_packets_count_ -= estimated_tcp_congestion_window_; + DCHECK_LT(0u, estimated_tcp_congestion_window_); + // With dynamic beta/alpha based on number of active streams, it is possible + // for the required_ack_count to become much lower than acked_packets_count_ + // suddenly, leading to more than one iteration through the following loop. + while (true) { + // Update estimated TCP congestion_window. + uint32 required_ack_count = + estimated_tcp_congestion_window_ / kNConnectionAlpha; + if (acked_packets_count_ < required_ack_count) { + break; + } + acked_packets_count_ -= required_ack_count; estimated_tcp_congestion_window_++; } + // Compute target congestion_window based on cubic target and estimated TCP // congestion_window, use highest (fastest). if (target_congestion_window < estimated_tcp_congestion_window_) { diff --git a/net/quic/congestion_control/cubic_test.cc b/net/quic/congestion_control/cubic_test.cc index 8e0d8e9..00bd37b 100644 --- a/net/quic/congestion_control/cubic_test.cc +++ b/net/quic/congestion_control/cubic_test.cc @@ -11,6 +11,12 @@ namespace net { namespace test { +const float kBeta = static_cast(0.7); // Default Cubic backoff factor. +const uint32 kNumConnections = 2; +const float kNConnectionBeta = (kNumConnections - 1 + kBeta) / kNumConnections; +const float kNConnectionAlpha = 3 * kNumConnections * kNumConnections * + (1 - kNConnectionBeta) / (1 + kNConnectionBeta); + class CubicTest : public ::testing::Test { protected: CubicTest() @@ -24,7 +30,7 @@ class CubicTest : public ::testing::Test { Cubic cubic_; }; -TEST_F(CubicTest, AboveOrgin) { +TEST_F(CubicTest, AboveOrigin) { // Convex growth. const QuicTime::Delta rtt_min = hundred_ms_; uint32 current_cwnd = 10; @@ -36,14 +42,14 @@ TEST_F(CubicTest, AboveOrgin) { current_cwnd = expected_cwnd; // Normal TCP phase. for (int i = 0; i < 48; ++i) { - for (uint32 n = 1; n < current_cwnd; ++n) { + for (uint32 n = 1; n < current_cwnd / kNConnectionAlpha; ++n) { // Call once per ACK. - EXPECT_EQ(current_cwnd, - cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min)); + EXPECT_NEAR(current_cwnd, + cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min), 1); } clock_.AdvanceTime(hundred_ms_); current_cwnd = cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min); - EXPECT_EQ(expected_cwnd, current_cwnd); + EXPECT_NEAR(expected_cwnd, current_cwnd, 1); expected_cwnd++; } // Cubic phase. @@ -70,15 +76,15 @@ TEST_F(CubicTest, LossEvents) { clock_.AdvanceTime(one_ms_); EXPECT_EQ(expected_cwnd, cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min)); - expected_cwnd = current_cwnd * 939 / 1024; + expected_cwnd = static_cast(current_cwnd * kNConnectionBeta); EXPECT_EQ(expected_cwnd, cubic_.CongestionWindowAfterPacketLoss(current_cwnd)); - expected_cwnd = current_cwnd * 939 / 1024; + expected_cwnd = static_cast(current_cwnd * kNConnectionBeta); EXPECT_EQ(expected_cwnd, cubic_.CongestionWindowAfterPacketLoss(current_cwnd)); } -TEST_F(CubicTest, BelowOrgin) { +TEST_F(CubicTest, BelowOrigin) { // Concave growth. const QuicTime::Delta rtt_min = hundred_ms_; uint32 current_cwnd = 422; @@ -87,23 +93,18 @@ TEST_F(CubicTest, BelowOrgin) { clock_.AdvanceTime(one_ms_); EXPECT_EQ(expected_cwnd, cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min)); - expected_cwnd = current_cwnd * 939 / 1024; + expected_cwnd = static_cast(current_cwnd * kNConnectionBeta); EXPECT_EQ(expected_cwnd, cubic_.CongestionWindowAfterPacketLoss(current_cwnd)); current_cwnd = expected_cwnd; - // First update after epoch. + // First update after loss to initialize the epoch. current_cwnd = cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min); // Cubic phase. - for (int i = 0; i < 54; ++i) { - for (uint32 n = 1; n < current_cwnd; ++n) { - // Call once per ACK. - EXPECT_EQ(current_cwnd, - cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min)); - } + for (int i = 0; i < 40 ; ++i) { clock_.AdvanceTime(hundred_ms_); current_cwnd = cubic_.CongestionWindowAfterAck(current_cwnd, rtt_min); } - expected_cwnd = 440; + expected_cwnd = 422; EXPECT_EQ(expected_cwnd, current_cwnd); } diff --git a/net/quic/congestion_control/tcp_cubic_sender.cc b/net/quic/congestion_control/tcp_cubic_sender.cc index a978cf6..b44d2da 100644 --- a/net/quic/congestion_control/tcp_cubic_sender.cc +++ b/net/quic/congestion_control/tcp_cubic_sender.cc @@ -24,7 +24,7 @@ const QuicByteCount kDefaultReceiveWindow = 64000; const int64 kInitialCongestionWindow = 10; const int kMaxBurstLength = 3; // Constants used for RTT calculation. -const int kInitialRttMs = 60; // At a typical RTT 60 ms. +const int kInitialRttMs = 100; // At a typical RTT 100 ms. const float kAlpha = 0.125f; const float kOneMinusAlpha = (1 - kAlpha); const float kBeta = 0.25f; diff --git a/net/quic/congestion_control/tcp_cubic_sender_test.cc b/net/quic/congestion_control/tcp_cubic_sender_test.cc index 9faecc4..88921ba 100644 --- a/net/quic/congestion_control/tcp_cubic_sender_test.cc +++ b/net/quic/congestion_control/tcp_cubic_sender_test.cc @@ -498,7 +498,9 @@ TEST_F(TcpCubicSenderTest, TcpRenoMaxCongestionWindow) { TEST_F(TcpCubicSenderTest, TcpCubicMaxCongestionWindow) { const QuicTcpCongestionWindow kMaxCongestionWindowTCP = 50; - const int kNumberOfAcks = 1000; + // Set to 10000 to compensate for small cubic alpha. + const int kNumberOfAcks = 10000; + sender_.reset( new TcpCubicSenderPeer(&clock_, false, kMaxCongestionWindowTCP)); diff --git a/net/quic/crypto/quic_crypto_server_config.cc b/net/quic/crypto/quic_crypto_server_config.cc index 8d8c203..e2b0583 100644 --- a/net/quic/crypto/quic_crypto_server_config.cc +++ b/net/quic/crypto/quic_crypto_server_config.cc @@ -147,6 +147,11 @@ class VerifyNonceIsValidAndUniqueCallback // static const char QuicCryptoServerConfig::TESTING[] = "secret string for testing"; +PrimaryConfigChangedCallback::PrimaryConfigChangedCallback() { +} + +PrimaryConfigChangedCallback::~PrimaryConfigChangedCallback() { +} ValidateClientHelloResultCallback::ValidateClientHelloResultCallback() { } @@ -416,6 +421,14 @@ bool QuicCryptoServerConfig::SetConfigs( return ok; } +void QuicCryptoServerConfig::GetConfigIds(vector* scids) const { + base::AutoLock locked(configs_lock_); + for (ConfigMap::const_iterator it = configs_.begin(); + it != configs_.end(); ++it) { + scids->push_back(it->first); + } +} + void QuicCryptoServerConfig::ValidateClientHello( const CryptoHandshakeMessage& client_hello, IPEndPoint client_ip, @@ -784,6 +797,9 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig( << base::HexEncode( reinterpret_cast(primary_config_->orbit), kOrbitSize); + if (primary_config_changed_cb_.get() != NULL) { + primary_config_changed_cb_->Run(primary_config_->id); + } return; } @@ -801,6 +817,9 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig( reinterpret_cast(primary_config_->orbit), kOrbitSize); next_config_promotion_time_ = QuicWallTime::Zero(); + if (primary_config_changed_cb_.get() != NULL) { + primary_config_changed_cb_->Run(primary_config_->id); + } } void QuicCryptoServerConfig::EvaluateClientHello( @@ -1184,6 +1203,12 @@ void QuicCryptoServerConfig::set_server_nonce_strike_register_window_secs( server_nonce_strike_register_window_secs_ = window_secs; } +void QuicCryptoServerConfig::AcquirePrimaryConfigChangedCb( + PrimaryConfigChangedCallback* cb) { + base::AutoLock locked(configs_lock_); + primary_config_changed_cb_.reset(cb); +} + string QuicCryptoServerConfig::NewSourceAddressToken( const IPEndPoint& ip, QuicRandom* rand, diff --git a/net/quic/crypto/quic_crypto_server_config.h b/net/quic/crypto/quic_crypto_server_config.h index 0886a52..0b76268 100644 --- a/net/quic/crypto/quic_crypto_server_config.h +++ b/net/quic/crypto/quic_crypto_server_config.h @@ -39,6 +39,14 @@ namespace test { class QuicCryptoServerConfigPeer; } // namespace test +// Hook that allows application code to subscribe to primary config changes. +class PrimaryConfigChangedCallback { + public: + PrimaryConfigChangedCallback(); + virtual ~PrimaryConfigChangedCallback(); + virtual void Run(const std::string& scid) = 0; +}; + // Callback used to accept the result of the |client_hello| validation step. class NET_EXPORT_PRIVATE ValidateClientHelloResultCallback { public: @@ -134,6 +142,9 @@ class NET_EXPORT_PRIVATE QuicCryptoServerConfig { bool SetConfigs(const std::vector& protobufs, QuicWallTime now); + // Get the server config ids for all known configs. + void GetConfigIds(std::vector* scids) const; + // Checks |client_hello| for gross errors and determines whether it // can be shown to be fresh (i.e. not a replay). The result of the // validation step must be interpreted by calling @@ -254,6 +265,9 @@ class NET_EXPORT_PRIVATE QuicCryptoServerConfig { // uniqueness. void set_server_nonce_strike_register_window_secs(uint32 window_secs); + // Set and take ownership of the callback to invoke on primary config changes. + void AcquirePrimaryConfigChangedCb(PrimaryConfigChangedCallback* cb); + private: friend class test::QuicCryptoServerConfigPeer; @@ -382,6 +396,8 @@ class NET_EXPORT_PRIVATE QuicCryptoServerConfig { // next_config_promotion_time_ contains the nearest, future time when an // active config will be promoted to primary. mutable QuicWallTime next_config_promotion_time_; + // Callback to invoke when the primary config changes. + scoped_ptr primary_config_changed_cb_; // Protects access to the pointer held by strike_register_client_. mutable base::Lock strike_register_client_lock_; diff --git a/net/quic/quic_client_session_test.cc b/net/quic/quic_client_session_test.cc index 8dd5041..5e0ea53 100644 --- a/net/quic/quic_client_session_test.cc +++ b/net/quic/quic_client_session_test.cc @@ -37,8 +37,7 @@ class TestPacketWriter : public QuicDefaultPacketWriter { virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE { + const IPEndPoint& peer_address) OVERRIDE { QuicFramer framer(QuicSupportedVersions(), QuicTime::Zero(), true); FramerVisitorCapturingFrames visitor; framer.set_visitor(&visitor); diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc index f1e25d6..737fee4 100644 --- a/net/quic/quic_connection.cc +++ b/net/quic/quic_connection.cc @@ -167,6 +167,7 @@ QuicConnection::QuicConnection(QuicGuid guid, largest_seen_packet_with_ack_(0), pending_version_negotiation_packet_(false), received_packet_manager_(kTCP), + ack_queued_(false), ack_alarm_(helper->CreateAlarm(new AckAlarm(this))), retransmission_alarm_(helper->CreateAlarm(new RetransmissionAlarm(this))), send_alarm_(helper->CreateAlarm(new SendAlarm(this))), @@ -636,13 +637,7 @@ void QuicConnection::OnPacketComplete() { << last_stream_frames_.size() << " stream frames for " << last_header_.public_header.guid; - // Must called before ack processing, because processing acks removes entries - // from unacket_packets_, increasing the least_unacked. - const bool last_packet_should_instigate_ack = ShouldLastPacketInstigateAck(); - - // If the incoming packet was missing, send an ack immediately. - bool send_ack_immediately = received_packet_manager_.IsMissing( - last_header_.packet_sequence_number); + MaybeQueueAck(); // Discard the packet if the visitor fails to process the stream frames. if (!last_stream_frames_.empty() && @@ -680,15 +675,36 @@ void QuicConnection::OnPacketComplete() { // If there are new missing packets to report, send an ack immediately. if (received_packet_manager_.HasNewMissingPackets()) { - send_ack_immediately = true; + ack_queued_ = true; + ack_alarm_->Cancel(); } - MaybeSendInResponseToPacket(send_ack_immediately, - last_packet_should_instigate_ack); - ClearLastFrames(); } +void QuicConnection::MaybeQueueAck() { + // If the incoming packet was missing, send an ack immediately. + ack_queued_ = received_packet_manager_.IsMissing( + last_header_.packet_sequence_number); + + // ShouldLastPacketInstigateAck must called before ack processing, because + // processing acks removes entries from unacket_packets_, increasing the + // least_unacked. + if (!ack_queued_ && ShouldLastPacketInstigateAck()) { + if (ack_alarm_->IsSet()) { + ack_queued_ = true; + } else { + ack_alarm_->Set(clock_->ApproximateNow().Add( + sent_packet_manager_.DelayedAckTime())); + DVLOG(1) << "Ack timer set; next packet or timer will trigger ACK."; + } + } + + if (ack_queued_) { + ack_alarm_->Cancel(); + } +} + void QuicConnection::ClearLastFrames() { last_stream_frames_.clear(); last_goaway_frames_.clear(); @@ -717,9 +733,8 @@ bool QuicConnection::ShouldLastPacketInstigateAck() { return true; } - // If the peer is still waiting for a packet that we are no - // longer planning to send, we should send an ack to raise - // the high water mark. + // If the peer is still waiting for a packet that we are no longer planning to + // send, send an ack to raise the high water mark. if (!last_ack_frames_.empty() && !last_ack_frames_.back().received_info.missing_packets.empty()) { return sent_packet_manager_.GetLeastUnackedSentPacket() > @@ -728,61 +743,57 @@ bool QuicConnection::ShouldLastPacketInstigateAck() { return false; } -void QuicConnection::MaybeSendInResponseToPacket( - bool send_ack_immediately, - bool last_packet_should_instigate_ack) { - // |include_ack| is false since we decide about ack bundling below. +void QuicConnection::MaybeSendInResponseToPacket() { + if (!connected_) { + return; + } ScopedPacketBundler bundler(this, false); - - if (last_packet_should_instigate_ack) { - // In general, we ack every second packet. When we don't ack the first - // packet, we set the delayed ack alarm. Thus, if the ack alarm is set - // then we know this is the second packet, and we should send an ack. - if (send_ack_immediately || ack_alarm_->IsSet()) { - SendAck(); - DCHECK(!ack_alarm_->IsSet()); - } else { - ack_alarm_->Set(clock_->ApproximateNow().Add( - sent_packet_manager_.DelayedAckTime())); - DVLOG(1) << "Ack timer set; next packet or timer will trigger ACK."; - } + if (ack_queued_) { + SendAck(); } - if (!last_ack_frames_.empty()) { - // Now the we have received an ack, we might be able to send packets which - // are queued locally, or drain streams which are blocked. - QuicTime::Delta delay = sent_packet_manager_.TimeUntilSend( - time_of_last_received_packet_, NOT_RETRANSMISSION, - HAS_RETRANSMITTABLE_DATA, NOT_HANDSHAKE); - if (delay.IsZero()) { - send_alarm_->Cancel(); - WriteIfNotBlocked(); - } else if (!delay.IsInfinite()) { - send_alarm_->Cancel(); - send_alarm_->Set(time_of_last_received_packet_.Add(delay)); - } + // Now that we have received an ack, we might be able to send packets which + // are queued locally, or drain streams which are blocked. + QuicTime::Delta delay = sent_packet_manager_.TimeUntilSend( + time_of_last_received_packet_, NOT_RETRANSMISSION, + HAS_RETRANSMITTABLE_DATA, NOT_HANDSHAKE); + if (delay.IsZero()) { + send_alarm_->Cancel(); + WriteIfNotBlocked(); + } else if (!delay.IsInfinite()) { + send_alarm_->Cancel(); + send_alarm_->Set(time_of_last_received_packet_.Add(delay)); } } void QuicConnection::SendVersionNegotiationPacket() { + // TODO(alyssar): implement zero server state negotiation. + pending_version_negotiation_packet_ = true; + if (writer_->IsWriteBlocked()) { + visitor_->OnWriteBlocked(); + return; + } scoped_ptr version_packet( packet_creator_.SerializeVersionNegotiationPacket( framer_.supported_versions())); - // TODO(satyamshekhar): implement zero server state negotiation. - WriteResult result = - writer_->WritePacket(version_packet->data(), version_packet->length(), - self_address().address(), peer_address(), this); - if (result.status == WRITE_STATUS_OK || - (result.status == WRITE_STATUS_BLOCKED && - writer_->IsWriteBlockedDataBuffered())) { - pending_version_negotiation_packet_ = false; - return; - } + WriteResult result = writer_->WritePacket( + version_packet->data(), version_packet->length(), + self_address().address(), peer_address()); + if (result.status == WRITE_STATUS_ERROR) { // We can't send an error as the socket is presumably borked. CloseConnection(QUIC_PACKET_WRITE_ERROR, false); + return; } - pending_version_negotiation_packet_ = true; + if (result.status == WRITE_STATUS_BLOCKED) { + visitor_->OnWriteBlocked(); + if (writer_->IsWriteBlockedDataBuffered()) { + pending_version_negotiation_packet_ = false; + } + return; + } + + pending_version_negotiation_packet_ = false; } QuicConsumedData QuicConnection::SendStreamData( @@ -873,8 +884,10 @@ void QuicConnection::ProcessUdpPacket(const IPEndPoint& self_address, << last_header_.packet_sequence_number; return; } + MaybeProcessUndecryptablePackets(); MaybeProcessRevivedPacket(); + MaybeSendInResponseToPacket(); } bool QuicConnection::OnCanWrite() { @@ -1027,6 +1040,7 @@ bool QuicConnection::CanWrite(TransmissionType transmission_type, HasRetransmittableData retransmittable, IsHandshake handshake) { if (writer_->IsWriteBlocked()) { + visitor_->OnWriteBlocked(); return false; } @@ -1107,6 +1121,7 @@ bool QuicConnection::WritePacket(EncryptionLevel level, // This assures we won't try to write *forced* packets when blocked. // Return true to stop processing. if (writer_->IsWriteBlocked()) { + visitor_->OnWriteBlocked(); return true; } } @@ -1141,7 +1156,7 @@ bool QuicConnection::WritePacket(EncryptionLevel level, WriteResult result = writer_->WritePacket(encrypted->data(), encrypted->length(), - self_address().address(), peer_address(), this); + self_address().address(), peer_address()); if (result.error_code == ERR_IO_PENDING) { DCHECK_EQ(WRITE_STATUS_BLOCKED, result.status); } @@ -1150,6 +1165,7 @@ bool QuicConnection::WritePacket(EncryptionLevel level, debug_visitor_->OnPacketSent(sequence_number, level, *encrypted, result); } if (result.status == WRITE_STATUS_BLOCKED) { + visitor_->OnWriteBlocked(); // If the socket buffers the the data, then the packet should not // be queued and sent again, which would result in an unnecessary // duplicate packet being sent. The helper must call OnPacketSent @@ -1195,25 +1211,12 @@ bool QuicConnection::ShouldDiscardPacket( return true; } - if (retransmittable == HAS_RETRANSMITTABLE_DATA) { - if (sent_packet_manager_.IsPreviousTransmission(sequence_number)) { - // If somehow we have already retransmitted this packet *before* - // we actually send it for the first time (I think this is probably - // impossible in the real world), then don't bother sending it. - // We don't want to call DiscardUnackedPacket because in this case - // the peer has not yet ACK'd the data. We need the subsequent - // retransmission to be sent. - DVLOG(1) << ENDPOINT << "Dropping packet: " << sequence_number - << " since it has already been retransmitted."; - return true; - } - - if (!sent_packet_manager_.HasRetransmittableFrames(sequence_number)) { - DVLOG(1) << ENDPOINT << "Dropping packet: " << sequence_number - << " since a previous transmission has been acked."; - sent_packet_manager_.DiscardUnackedPacket(sequence_number); - return true; - } + if (retransmittable == HAS_RETRANSMITTABLE_DATA && + !sent_packet_manager_.HasRetransmittableFrames(sequence_number)) { + DVLOG(1) << ENDPOINT << "Dropping packet: " << sequence_number + << " since a previous transmission has been acked."; + sent_packet_manager_.DiscardUnackedPacket(sequence_number); + return true; } return false; diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h index c7b34b7..0b35c72 100644 --- a/net/quic/quic_connection.h +++ b/net/quic/quic_connection.h @@ -427,25 +427,6 @@ class NET_EXPORT_PRIVATE QuicConnection const SerializedPacket& packet, TransmissionType transmission_type); - // Writes the given packet to socket, encrypted with |level|, with the help - // of helper. Returns true on successful write, false otherwise. However, - // behavior is undefined if connection is not established or broken. In any - // circumstances, a return value of true implies that |packet| has been - // transmitted and may be destroyed. If |sequence_number| is present in - // |retransmission_map_| it also sets up retransmission of the given packet - // in case of successful write. If |force| is FORCE, then the packet will be - // sent immediately and the send scheduler will not be consulted. - bool WritePacket(EncryptionLevel level, - QuicPacketSequenceNumber sequence_number, - const QuicPacket& packet, - TransmissionType transmission_type, - HasRetransmittableData retransmittable, - IsHandshake handshake, - Force force); - - // Make sure an ack we got from our peer is sane. - bool ValidateAckFrame(const QuicAckFrame& incoming_ack); - QuicConnectionHelperInterface* helper() { return helper_; } // Selects and updates the version of the protocol being used by selecting a @@ -453,8 +434,6 @@ class NET_EXPORT_PRIVATE QuicConnection // such a version exists, false otherwise. bool SelectMutualVersion(const QuicVersionVector& available_versions); - QuicFramer framer_; - private: // Stores current batch state for connection, puts the connection // into batch mode, and destruction restores the stored batch state. @@ -532,6 +511,24 @@ class NET_EXPORT_PRIVATE QuicConnection typedef std::list QueuedPacketList; typedef std::map FecGroupMap; + // Writes the given packet to socket, encrypted with |level|. Returns true on + // successful write. Behavior is undefined if connection is not established or + // broken. In any circumstance, a return value of true implies that |packet| + // has been transmitted and may be destroyed. If |sequence_number| is present + // in |retransmission_map_| it also sets up retransmission of the given packet + // in case of successful write. If |force| is FORCE, then the packet will be + // sent immediately and the send scheduler will not be consulted. + bool WritePacket(EncryptionLevel level, + QuicPacketSequenceNumber sequence_number, + const QuicPacket& packet, + TransmissionType transmission_type, + HasRetransmittableData retransmittable, + IsHandshake handshake, + Force force); + + // Make sure an ack we got from our peer is sane. + bool ValidateAckFrame(const QuicAckFrame& incoming_ack); + // Sends a version negotiation packet to the peer. void SendVersionNegotiationPacket(); @@ -578,13 +575,16 @@ class NET_EXPORT_PRIVATE QuicConnection // Update the |sent_info| for an outgoing ack. void UpdateSentPacketInfo(SentPacketInfo* sent_info); + // Queues an ack or sets the ack alarm when an incoming packet arrives that + // should be acked. + void MaybeQueueAck(); + // Checks if the last packet should instigate an ack. bool ShouldLastPacketInstigateAck(); // Sends any packets which are a response to the last packet, including both // acks and pending writes if an ack opened the congestion window. - void MaybeSendInResponseToPacket(bool send_ack_immediately, - bool last_packet_should_instigate_ack); + void MaybeSendInResponseToPacket(); // Get the FEC group associate with the last processed packet or NULL, if the // group has already been deleted. @@ -593,6 +593,7 @@ class NET_EXPORT_PRIVATE QuicConnection // Closes any FEC groups protecting packets before |sequence_number|. void CloseFecGroupsBefore(QuicPacketSequenceNumber sequence_number); + QuicFramer framer_; QuicConnectionHelperInterface* helper_; // Not owned. QuicPacketWriter* writer_; // Not owned. EncryptionLevel encryption_level_; @@ -647,6 +648,9 @@ class NET_EXPORT_PRIVATE QuicConnection QuicReceivedPacketManager received_packet_manager_; QuicSentEntropyManager sent_entropy_manager_; + // Indicates whether an ack should be sent the next time we try to write. + bool ack_queued_; + // An alarm that fires when an ACK should be sent to the peer. scoped_ptr ack_alarm_; // An alarm that fires when a packet needs to be retransmitted. diff --git a/net/quic/quic_connection_test.cc b/net/quic/quic_connection_test.cc index e2b0fef..48aedd6 100644 --- a/net/quic/quic_connection_test.cc +++ b/net/quic/quic_connection_test.cc @@ -31,6 +31,7 @@ using std::map; using std::vector; using testing::_; using testing::AnyNumber; +using testing::AtLeast; using testing::ContainerEq; using testing::Contains; using testing::DoAll; @@ -274,7 +275,7 @@ class TestPacketWriter : public QuicPacketWriter { TestPacketWriter() : last_packet_size_(0), write_blocked_(false), - block_next_write_(false), + block_on_next_write_(false), is_write_blocked_data_buffered_(false), is_server_(true), final_bytes_of_last_packet_(0), @@ -287,8 +288,7 @@ class TestPacketWriter : public QuicPacketWriter { virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE { + const IPEndPoint& peer_address) OVERRIDE { QuicEncryptedPacket packet(buffer, buf_len); ++packets_write_attempts_; @@ -305,9 +305,9 @@ class TestPacketWriter : public QuicPacketWriter { visitor_.Reset(); framer.set_visitor(&visitor_); EXPECT_TRUE(framer.ProcessPacket(packet)); - if (block_next_write_) { + if (block_on_next_write_) { write_blocked_ = true; - block_next_write_ = false; + block_on_next_write_ = false; } if (IsWriteBlocked()) { return WriteResult(WRITE_STATUS_BLOCKED, -1); @@ -324,7 +324,7 @@ class TestPacketWriter : public QuicPacketWriter { virtual void SetWritable() OVERRIDE { write_blocked_ = false; } - void BlockNextWrite() { block_next_write_ = true; } + void BlockOnNextWrite() { block_on_next_write_ = true; } // Resets the visitor's state by clearing out the headers and frames. void Reset() { @@ -380,7 +380,7 @@ class TestPacketWriter : public QuicPacketWriter { FramerVisitorCapturingFrames visitor_; size_t last_packet_size_; bool write_blocked_; - bool block_next_write_; + bool block_on_next_write_; bool is_write_blocked_data_buffered_; bool is_server_; uint32 final_bytes_of_last_packet_; @@ -472,7 +472,7 @@ class TestConnection : public QuicConnection { } void set_version(QuicVersion version) { - framer_.set_version(version); + QuicConnectionPeer::GetFramer(this)->set_version(version); } void set_is_server(bool is_server) { @@ -841,6 +841,11 @@ class QuicConnectionTest : public ::testing::TestWithParam { QuicConnectionPeer::GetConnectionClosePacket(&connection_) == NULL); } + void BlockOnNextWrite() { + writer_->BlockOnNextWrite(); + EXPECT_CALL(visitor_, OnWriteBlocked()).Times(AtLeast(1)); + } + QuicGuid guid_; QuicFramer framer_; QuicPacketCreator creator_; @@ -1104,8 +1109,9 @@ TEST_F(QuicConnectionTest, LeastUnackedLower) { // This should be fine. creator_.set_sequence_number(1); QuicAckFrame frame2 = InitAckFrame(0, 1); - // The scheduler will not process out of order acks. - EXPECT_CALL(visitor_, OnCanWrite()).Times(0); + // The scheduler will not process out of order acks, but all packet processing + // causes the connection to try to write. + EXPECT_CALL(visitor_, OnCanWrite()).Times(1); ProcessAckPacket(&frame2); // Now claim it's one, but set the ordering so it was sent "after" the first @@ -1257,8 +1263,6 @@ TEST_F(QuicConnectionTest, SendingDifferentSequenceNumberLengthsUnackedDelta) { TEST_F(QuicConnectionTest, BasicSending) { EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); -// EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); -// EXPECT_CALL(*send_algorithm_, OnPacketAcked(_, _)).Times(6); QuicPacketSequenceNumber last_packet; SendStreamDataToPeer(1, "foo", 0, !kFin, &last_packet); // Packet 1 EXPECT_EQ(1u, last_packet); @@ -1340,7 +1344,7 @@ TEST_F(QuicConnectionTest, FECQueueing) { connection_.options()->max_packets_per_fec_group = 2; EXPECT_EQ(0u, connection_.NumQueuedPackets()); - writer_->BlockNextWrite(); + BlockOnNextWrite(); const string payload(payload_length, 'a'); connection_.SendStreamDataWithString(1, payload, 0, !kFin, NULL); EXPECT_FALSE(creator_.ShouldSendFec(true)); @@ -1610,7 +1614,7 @@ TEST_F(QuicConnectionTest, FramePackingSendvQueued) { // Try to send two stream frames in 1 packet by using writev. EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, NOT_RETRANSMISSION, _)); - writer_->BlockNextWrite(); + BlockOnNextWrite(); char data[] = "ABCD"; IOVector data_iov; data_iov.AppendNoCoalesce(data, 2); @@ -1729,10 +1733,9 @@ TEST_F(QuicConnectionTest, DiscardRetransmit) { NackPacket(2, &nack_two); // The first nack should trigger a fast retransmission, but we'll be // write blocked, so the packet will be queued. - writer_->BlockNextWrite(); + BlockOnNextWrite(); EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); EXPECT_CALL(*send_algorithm_, OnPacketAcked(_, _)); - ProcessAckPacket(&nack_two); EXPECT_EQ(1u, connection_.NumQueuedPackets()); @@ -1779,7 +1782,7 @@ TEST_F(QuicConnectionTest, QueueAfterTwoRTOs) { } // Block the congestion window and ensure they're queued. - writer_->BlockNextWrite(); + BlockOnNextWrite(); clock_.AdvanceTime(DefaultRetransmissionTime()); // Only one packet should be retransmitted. EXPECT_CALL(*send_algorithm_, OnRetransmissionTimeout(true)); @@ -1798,9 +1801,8 @@ TEST_F(QuicConnectionTest, QueueAfterTwoRTOs) { } TEST_F(QuicConnectionTest, WriteBlockedThenSent) { - writer_->BlockNextWrite(); + BlockOnNextWrite(); writer_->set_is_write_blocked_data_buffered(true); - connection_.SendStreamDataWithString(1, "foo", 0, !kFin, NULL); EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -1811,8 +1813,7 @@ TEST_F(QuicConnectionTest, WriteBlockedThenSent) { TEST_F(QuicConnectionTest, WriteBlockedAckedThenSent) { EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); - writer_->BlockNextWrite(); - + BlockOnNextWrite(); writer_->set_is_write_blocked_data_buffered(true); connection_.SendStreamDataWithString(1, "foo", 0, !kFin, NULL); EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -1832,9 +1833,8 @@ TEST_F(QuicConnectionTest, RetransmitWriteBlockedAckedOriginalThenSent) { connection_.SendStreamDataWithString(3, "foo", 0, !kFin, NULL); EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); - writer_->BlockNextWrite(); + BlockOnNextWrite(); writer_->set_is_write_blocked_data_buffered(true); - // Simulate the retransmission alarm firing. EXPECT_CALL(*send_algorithm_, OnRetransmissionTimeout(_)); clock_.AdvanceTime(DefaultRetransmissionTime()); @@ -1852,7 +1852,7 @@ TEST_F(QuicConnectionTest, RetransmitWriteBlockedAckedOriginalThenSent) { TEST_F(QuicConnectionTest, ResumptionAlarmWhenWriteBlocked) { // Block the connection. - writer_->BlockNextWrite(); + BlockOnNextWrite(); connection_.SendStreamDataWithString(3, "foo", 0, !kFin, NULL); EXPECT_EQ(1u, writer_->packets_write_attempts()); EXPECT_TRUE(writer_->IsWriteBlocked()); @@ -2255,7 +2255,7 @@ TEST_F(QuicConnectionTest, RetransmissionCountCalculation) { } TEST_F(QuicConnectionTest, SetRTOAfterWritingToSocket) { - writer_->BlockNextWrite(); + BlockOnNextWrite(); connection_.SendStreamDataWithString(1, "foo", 0, !kFin, NULL); // Make sure that RTO is not started when the packet is queued. EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -2306,7 +2306,7 @@ TEST_F(QuicConnectionTest, DelayRTOWithAckReceipt) { TEST_F(QuicConnectionTest, TestQueued) { EXPECT_EQ(0u, connection_.NumQueuedPackets()); - writer_->BlockNextWrite(); + BlockOnNextWrite(); connection_.SendStreamDataWithString(1, "foo", 0, !kFin, NULL); EXPECT_EQ(1u, connection_.NumQueuedPackets()); @@ -2461,7 +2461,7 @@ TEST_F(QuicConnectionTest, SendSchedulerForce) { TEST_F(QuicConnectionTest, SendSchedulerEAGAIN) { QuicPacket* packet = ConstructDataPacket(1, 0, !kEntropyFlag); - writer_->BlockNextWrite(); + BlockOnNextWrite(); EXPECT_CALL(*send_algorithm_, TimeUntilSend(_, NOT_RETRANSMISSION, _, _)).WillOnce( testing::Return(QuicTime::Delta::Zero())); @@ -2936,7 +2936,7 @@ TEST_F(QuicConnectionTest, ServerSendsVersionNegotiationPacketSocketBlocked) { framer_.set_version(QuicVersionMax()); connection_.set_is_server(true); - writer_->BlockNextWrite(); + BlockOnNextWrite(); connection_.ProcessUdpPacket(IPEndPoint(), IPEndPoint(), *encrypted); EXPECT_EQ(0u, writer_->last_packet_size()); EXPECT_TRUE(connection_.HasQueuedData()); @@ -2980,7 +2980,7 @@ TEST_F(QuicConnectionTest, framer_.set_version(QuicVersionMax()); connection_.set_is_server(true); - writer_->BlockNextWrite(); + BlockOnNextWrite(); writer_->set_is_write_blocked_data_buffered(true); connection_.ProcessUdpPacket(IPEndPoint(), IPEndPoint(), *encrypted); EXPECT_EQ(0u, writer_->last_packet_size()); @@ -3258,14 +3258,14 @@ TEST_F(QuicConnectionTest, ConnectionCloseWhenWritable) { } TEST_F(QuicConnectionTest, ConnectionCloseGettingWriteBlocked) { - writer_->BlockNextWrite(); + BlockOnNextWrite(); TriggerConnectionClose(); EXPECT_EQ(1u, writer_->packets_write_attempts()); EXPECT_TRUE(writer_->IsWriteBlocked()); } TEST_F(QuicConnectionTest, ConnectionCloseWhenWriteBlocked) { - writer_->BlockNextWrite(); + BlockOnNextWrite(); connection_.SendStreamDataWithString(1, "foo", 0, !kFin, NULL); EXPECT_EQ(1u, connection_.NumQueuedPackets()); EXPECT_EQ(1u, writer_->packets_write_attempts()); diff --git a/net/quic/quic_default_packet_writer.cc b/net/quic/quic_default_packet_writer.cc index e249ff1..5dd80c6 100644 --- a/net/quic/quic_default_packet_writer.cc +++ b/net/quic/quic_default_packet_writer.cc @@ -26,8 +26,7 @@ QuicDefaultPacketWriter::~QuicDefaultPacketWriter() {} WriteResult QuicDefaultPacketWriter::WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) { + const net::IPEndPoint& peer_address) { scoped_refptr buf( new StringIOBuffer(std::string(buffer, buf_len))); DCHECK(!IsWriteBlocked()); diff --git a/net/quic/quic_default_packet_writer.h b/net/quic/quic_default_packet_writer.h index 9254360..affc6f1 100644 --- a/net/quic/quic_default_packet_writer.h +++ b/net/quic/quic_default_packet_writer.h @@ -15,7 +15,7 @@ namespace net { -class QuicBlockedWriterInterface; +struct WriteResult; // Chrome specific packet writer which uses a DatagramClientSocket for writing // data. @@ -29,8 +29,7 @@ class NET_EXPORT_PRIVATE QuicDefaultPacketWriter : public QuicPacketWriter { virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE; + const net::IPEndPoint& peer_address) OVERRIDE; virtual bool IsWriteBlockedDataBuffered() const OVERRIDE; virtual bool IsWriteBlocked() const OVERRIDE; virtual void SetWritable() OVERRIDE; diff --git a/net/quic/quic_framer.cc b/net/quic/quic_framer.cc index 9d15c3c..964032c 100644 --- a/net/quic/quic_framer.cc +++ b/net/quic/quic_framer.cc @@ -311,6 +311,7 @@ SerializedPacket QuicFramer::BuildDataPacket( const SerializedPacket kNoPacket( 0, PACKET_1BYTE_SEQUENCE_NUMBER, NULL, 0, NULL); if (!AppendPacketHeader(header, &writer)) { + LOG(DFATAL) << "AppendPacketHeader failed"; return kNoPacket; } @@ -319,6 +320,7 @@ SerializedPacket QuicFramer::BuildDataPacket( const bool last_frame_in_packet = i == (frames.size() - 1); if (!AppendTypeByte(frame, last_frame_in_packet, &writer)) { + LOG(DFATAL) << "AppendTypeByte failed"; return kNoPacket; } @@ -327,41 +329,48 @@ SerializedPacket QuicFramer::BuildDataPacket( writer.WritePadding(); break; case STREAM_FRAME: - if (!AppendStreamFramePayload( + if (!AppendStreamFrame( *frame.stream_frame, last_frame_in_packet, &writer)) { + LOG(DFATAL) << "AppendStreamFrame failed"; return kNoPacket; } break; case ACK_FRAME: - if (!AppendAckFramePayloadAndTypeByte( + if (!AppendAckFrameAndTypeByte( header, *frame.ack_frame, &writer)) { + LOG(DFATAL) << "AppendAckFrameAndTypeByte failed"; return kNoPacket; } break; case CONGESTION_FEEDBACK_FRAME: - if (!AppendQuicCongestionFeedbackFramePayload( + if (!AppendQuicCongestionFeedbackFrame( *frame.congestion_feedback_frame, &writer)) { + LOG(DFATAL) << "AppendQuicCongestionFeedbackFrame failed"; return kNoPacket; } break; case RST_STREAM_FRAME: - if (!AppendRstStreamFramePayload(*frame.rst_stream_frame, &writer)) { + if (!AppendRstStreamFrame(*frame.rst_stream_frame, &writer)) { + LOG(DFATAL) << "AppendRstStreamFrame failed"; return kNoPacket; } break; case CONNECTION_CLOSE_FRAME: - if (!AppendConnectionCloseFramePayload( + if (!AppendConnectionCloseFrame( *frame.connection_close_frame, &writer)) { + LOG(DFATAL) << "AppendConnectionCloseFrame failed"; return kNoPacket; } break; case GOAWAY_FRAME: - if (!AppendGoAwayFramePayload(*frame.goaway_frame, &writer)) { + if (!AppendGoAwayFrame(*frame.goaway_frame, &writer)) { + LOG(DFATAL) << "AppendGoAwayFrame failed"; return kNoPacket; } break; default: RaiseError(QUIC_INVALID_FRAME_DATA); + LOG(DFATAL) << "QUIC_INVALID_FRAME_DATA"; return kNoPacket; } } @@ -397,10 +406,12 @@ SerializedPacket QuicFramer::BuildFecPacket(const QuicPacketHeader& header, const SerializedPacket kNoPacket( 0, PACKET_1BYTE_SEQUENCE_NUMBER, NULL, 0, NULL); if (!AppendPacketHeader(header, &writer)) { + LOG(DFATAL) << "AppendPacketHeader failed"; return kNoPacket; } if (!writer.WriteBytes(fec.redundancy.data(), fec.redundancy.length())) { + LOG(DFATAL) << "Failed to add FEC"; return kNoPacket; } @@ -619,6 +630,7 @@ bool QuicFramer::ProcessRevivedPacket(QuicPacketHeader* header, bool QuicFramer::AppendPacketHeader(const QuicPacketHeader& header, QuicDataWriter* writer) { + DVLOG(1) << "Appending header: " << header; DCHECK(header.fec_group > 0 || header.is_in_fec_group == NOT_IN_FEC_GROUP); uint8 public_flags = 0; if (header.public_header.reset_flag) { @@ -1738,7 +1750,7 @@ bool QuicFramer::AppendPacketSequenceNumber( } } -bool QuicFramer::AppendStreamFramePayload( +bool QuicFramer::AppendStreamFrame( const QuicStreamFrame& frame, bool last_frame_in_packet, QuicDataWriter* writer) { @@ -1766,7 +1778,7 @@ void QuicFramer::set_version(const QuicVersion version) { quic_version_ = version; } -bool QuicFramer::AppendAckFramePayloadAndTypeByte( +bool QuicFramer::AppendAckFrameAndTypeByte( const QuicPacketHeader& header, const QuicAckFrame& frame, QuicDataWriter* writer) { @@ -1901,7 +1913,7 @@ bool QuicFramer::AppendAckFramePayloadAndTypeByte( return true; } -bool QuicFramer::AppendQuicCongestionFeedbackFramePayload( +bool QuicFramer::AppendQuicCongestionFeedbackFrame( const QuicCongestionFeedbackFrame& frame, QuicDataWriter* writer) { if (!writer->WriteBytes(&frame.type, 1)) { @@ -1991,7 +2003,7 @@ bool QuicFramer::AppendQuicCongestionFeedbackFramePayload( return true; } -bool QuicFramer::AppendRstStreamFramePayload( +bool QuicFramer::AppendRstStreamFrame( const QuicRstStreamFrame& frame, QuicDataWriter* writer) { if (!writer->WriteUInt32(frame.stream_id)) { @@ -2009,7 +2021,7 @@ bool QuicFramer::AppendRstStreamFramePayload( return true; } -bool QuicFramer::AppendConnectionCloseFramePayload( +bool QuicFramer::AppendConnectionCloseFrame( const QuicConnectionCloseFrame& frame, QuicDataWriter* writer) { uint32 error_code = static_cast(frame.error_code); @@ -2022,8 +2034,8 @@ bool QuicFramer::AppendConnectionCloseFramePayload( return true; } -bool QuicFramer::AppendGoAwayFramePayload(const QuicGoAwayFrame& frame, - QuicDataWriter* writer) { +bool QuicFramer::AppendGoAwayFrame(const QuicGoAwayFrame& frame, + QuicDataWriter* writer) { uint32 error_code = static_cast(frame.error_code); if (!writer->WriteUInt32(error_code)) { return false; diff --git a/net/quic/quic_framer.h b/net/quic/quic_framer.h index 47cbb96..5d15411 100644 --- a/net/quic/quic_framer.h +++ b/net/quic/quic_framer.h @@ -430,22 +430,20 @@ class NET_EXPORT_PRIVATE QuicFramer { bool AppendTypeByte(const QuicFrame& frame, bool last_frame_in_packet, QuicDataWriter* writer); - bool AppendStreamFramePayload(const QuicStreamFrame& frame, - bool last_frame_in_packet, - QuicDataWriter* builder); - bool AppendAckFramePayloadAndTypeByte(const QuicPacketHeader& header, - const QuicAckFrame& frame, - QuicDataWriter* builder); - bool AppendQuicCongestionFeedbackFramePayload( + bool AppendStreamFrame(const QuicStreamFrame& frame, + bool last_frame_in_packet, + QuicDataWriter* builder); + bool AppendAckFrameAndTypeByte(const QuicPacketHeader& header, + const QuicAckFrame& frame, + QuicDataWriter* builder); + bool AppendQuicCongestionFeedbackFrame( const QuicCongestionFeedbackFrame& frame, QuicDataWriter* builder); - bool AppendRstStreamFramePayload(const QuicRstStreamFrame& frame, - QuicDataWriter* builder); - bool AppendConnectionCloseFramePayload( - const QuicConnectionCloseFrame& frame, - QuicDataWriter* builder); - bool AppendGoAwayFramePayload(const QuicGoAwayFrame& frame, - QuicDataWriter* writer); + bool AppendRstStreamFrame(const QuicRstStreamFrame& frame, + QuicDataWriter* builder); + bool AppendConnectionCloseFrame(const QuicConnectionCloseFrame& frame, + QuicDataWriter* builder); + bool AppendGoAwayFrame(const QuicGoAwayFrame& frame, QuicDataWriter* writer); bool RaiseError(QuicErrorCode error); void set_error(QuicErrorCode error) { diff --git a/net/quic/quic_framer_test.cc b/net/quic/quic_framer_test.cc index b1ceb49..a45f376 100644 --- a/net/quic/quic_framer_test.cc +++ b/net/quic/quic_framer_test.cc @@ -20,6 +20,7 @@ #include "net/quic/quic_utils.h" #include "net/quic/test_tools/quic_framer_peer.h" #include "net/quic/test_tools/quic_test_utils.h" +#include "net/test/gtest_util.h" using base::hash_set; using base::StringPiece; @@ -2978,8 +2979,10 @@ TEST_P(QuicFramerTest, BuildCongestionFeedbackFramePacketInvalidFeedback) { QuicFrames frames; frames.push_back(QuicFrame(&congestion_feedback_frame)); - scoped_ptr data( - framer_.BuildUnsizedDataPacket(header, frames).packet); + scoped_ptr data; + EXPECT_DFATAL( + data.reset(framer_.BuildUnsizedDataPacket(header, frames).packet), + "AppendQuicCongestionFeedbackFrame failed"); ASSERT_TRUE(data == NULL); } diff --git a/net/quic/quic_http_stream_test.cc b/net/quic/quic_http_stream_test.cc index a49d6bb..926ea2a 100644 --- a/net/quic/quic_http_stream_test.cc +++ b/net/quic/quic_http_stream_test.cc @@ -263,6 +263,13 @@ class QuicHttpStreamTest : public ::testing::TestWithParam { sequence_number, !kIncludeVersion, stream_id_, QUIC_STREAM_CANCELLED); } + scoped_ptr ConstructAckAndRstStreamPacket( + QuicPacketSequenceNumber sequence_number) { + return maker_.MakeAckAndRstPacket( + sequence_number, !kIncludeVersion, stream_id_, QUIC_STREAM_CANCELLED, + 1, 1, !kIncludeCongestionFeedback); + } + scoped_ptr ConstructAckPacket( QuicPacketSequenceNumber sequence_number, QuicPacketSequenceNumber largest_received, @@ -611,7 +618,7 @@ TEST_P(QuicHttpStreamTest, DestroyedEarly) { } else { AddWrite(ConstructDataPacket(1, kIncludeVersion, kFin, 0, request_data_)); } - AddWrite(ConstructRstStreamPacket(2)); + AddWrite(ConstructAckAndRstStreamPacket(2)); use_closing_stream_ = true; Initialize(); @@ -648,7 +655,7 @@ TEST_P(QuicHttpStreamTest, Priority) { } else { AddWrite(ConstructDataPacket(1, kIncludeVersion, kFin, 0, request_data_)); } - AddWrite(ConstructRstStreamPacket(2)); + AddWrite(ConstructAckAndRstStreamPacket(2)); use_closing_stream_ = true; Initialize(); diff --git a/net/quic/quic_packet_creator.cc b/net/quic/quic_packet_creator.cc index 2ac8747..419404e 100644 --- a/net/quic/quic_packet_creator.cc +++ b/net/quic/quic_packet_creator.cc @@ -421,6 +421,7 @@ bool QuicPacketCreator::ShouldRetransmit(const QuicFrame& frame) { bool QuicPacketCreator::AddFrame(const QuicFrame& frame, bool save_retransmittable_frames) { + DVLOG(1) << "Adding frame: " << frame; size_t frame_len = framer_->GetSerializedFrameLength( frame, BytesFree(), queued_frames_.empty(), true, options()->send_sequence_number_length); diff --git a/net/quic/quic_packet_writer.h b/net/quic/quic_packet_writer.h index a4b26d6c..16b7ade 100644 --- a/net/quic/quic_packet_writer.h +++ b/net/quic/quic_packet_writer.h @@ -10,7 +10,6 @@ namespace net { -class QuicBlockedWriterInterface; struct WriteResult; // An interface between writers and the entity managing the @@ -27,8 +26,7 @@ class NET_EXPORT_PRIVATE QuicPacketWriter { virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) = 0; + const IPEndPoint& peer_address) = 0; // Returns true if the writer buffers and subsequently rewrites data // when an attempt to write results in the underlying socket becoming diff --git a/net/quic/quic_protocol.cc b/net/quic/quic_protocol.cc index a45c4af..69b0205 100644 --- a/net/quic/quic_protocol.cc +++ b/net/quic/quic_protocol.cc @@ -162,7 +162,7 @@ QuicVersion QuicTagToQuicVersion(const QuicTag version_tag) { } // Reading from the client so this should not be considered an ERROR. DVLOG(1) << "Unsupported QuicTag version: " - << QuicUtils::TagToString(version_tag); + << QuicUtils::TagToString(version_tag); return QUIC_VERSION_UNSUPPORTED; } @@ -275,6 +275,83 @@ QuicCongestionFeedbackFrame::QuicCongestionFeedbackFrame() { QuicCongestionFeedbackFrame::~QuicCongestionFeedbackFrame() { } +ostream& operator<<(ostream& os, const QuicFrame& frame) { + switch (frame.type) { + case PADDING_FRAME: { + os << "type { PADDING_FRAME } "; + break; + } + case RST_STREAM_FRAME: { + os << "type { " << RST_STREAM_FRAME << " } " << *(frame.rst_stream_frame); + break; + } + case CONNECTION_CLOSE_FRAME: { + os << "type { CONNECTION_CLOSE_FRAME } " + << *(frame.connection_close_frame); + break; + } + case GOAWAY_FRAME: { + os << "type { GOAWAY_FRAME } " << *(frame.goaway_frame); + break; + } + case STREAM_FRAME: { + os << "type { STREAM_FRAME } " << *(frame.stream_frame); + break; + } + case ACK_FRAME: { + os << "type { ACK_FRAME } " << *(frame.ack_frame); + break; + } + case CONGESTION_FEEDBACK_FRAME: { + os << "type { CONGESTION_FEEDBACK_FRAME } " + << *(frame.congestion_feedback_frame); + break; + } + default: { + LOG(ERROR) << "Unknown frame type: " << frame.type; + break; + } + } + return os; +} + +ostream& operator<<(ostream& os, const QuicRstStreamFrame& rst_frame) { + os << "stream_id { " << rst_frame.stream_id << " } " + << "error_code { " << rst_frame.error_code << " } " + << "error_details { " << rst_frame.error_details << " }\n"; + return os; +} + +ostream& operator<<(ostream& os, + const QuicConnectionCloseFrame& connection_close_frame) { + os << "error_code { " << connection_close_frame.error_code << " } " + << "error_details { " << connection_close_frame.error_details << " }\n"; + return os; +} + +ostream& operator<<(ostream& os, const QuicGoAwayFrame& goaway_frame) { + os << "error_code { " << goaway_frame.error_code << " } " + << "last_good_stream_id { " << goaway_frame.last_good_stream_id << " } " + << "reason_phrase { " << goaway_frame.reason_phrase << " }\n"; + return os; +} + +ostream& operator<<(ostream& os, const QuicStreamFrame& stream_frame) { + os << "stream_id { " << stream_frame.stream_id << " } " + << "fin { " << stream_frame.fin << " } " + << "offset { " << stream_frame.offset << " } " + << "data { " + << QuicUtils::StringToHexASCIIDump(*(stream_frame.GetDataAsString())) + << " }\n"; + return os; +} + +ostream& operator<<(ostream& os, const QuicAckFrame& ack_frame) { + os << "sent info { " << ack_frame.sent_info << " } " + << "received info { " << ack_frame.received_info << " }\n"; + return os; +} + ostream& operator<<(ostream& os, const QuicCongestionFeedbackFrame& congestion_frame) { os << "type: " << congestion_frame.type; @@ -309,12 +386,6 @@ ostream& operator<<(ostream& os, return os; } -ostream& operator<<(ostream& os, const QuicAckFrame& ack_frame) { - os << "sent info { " << ack_frame.sent_info << " } " - << "received info { " << ack_frame.received_info << " }\n"; - return os; -} - CongestionFeedbackMessageFixRate::CongestionFeedbackMessageFixRate() : bitrate(QuicBandwidth::Zero()) { } diff --git a/net/quic/quic_protocol.h b/net/quic/quic_protocol.h index 1352c34..dc88446 100644 --- a/net/quic/quic_protocol.h +++ b/net/quic/quic_protocol.h @@ -537,6 +537,9 @@ struct NET_EXPORT_PRIVATE QuicStreamFrame { QuicStreamOffset offset, IOVector data); + NET_EXPORT_PRIVATE friend std::ostream& operator<<( + std::ostream& os, const QuicStreamFrame& s); + // Returns a copy of the IOVector |data| as a heap-allocated string. // Caller must take ownership of the returned string. std::string* GetDataAsString() const; @@ -681,12 +684,18 @@ struct NET_EXPORT_PRIVATE QuicRstStreamFrame { DCHECK_LE(error_code, std::numeric_limits::max()); } + NET_EXPORT_PRIVATE friend std::ostream& operator<<( + std::ostream& os, const QuicRstStreamFrame& r); + QuicStreamId stream_id; QuicRstStreamErrorCode error_code; std::string error_details; }; struct NET_EXPORT_PRIVATE QuicConnectionCloseFrame { + NET_EXPORT_PRIVATE friend std::ostream& operator<<( + std::ostream& os, const QuicConnectionCloseFrame& c); + QuicErrorCode error_code; std::string error_details; }; @@ -697,6 +706,9 @@ struct NET_EXPORT_PRIVATE QuicGoAwayFrame { QuicStreamId last_good_stream_id, const std::string& reason); + NET_EXPORT_PRIVATE friend std::ostream& operator<<( + std::ostream& os, const QuicGoAwayFrame& g); + QuicErrorCode error_code; QuicStreamId last_good_stream_id; std::string reason_phrase; @@ -745,6 +757,9 @@ struct NET_EXPORT_PRIVATE QuicFrame { goaway_frame(frame) { } + NET_EXPORT_PRIVATE friend std::ostream& operator<<( + std::ostream& os, const QuicFrame& frame); + QuicFrameType type; union { QuicPaddingFrame* padding_frame; diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc index bb833bc..9d64131 100644 --- a/net/quic/quic_sent_packet_manager.cc +++ b/net/quic/quic_sent_packet_manager.cc @@ -173,8 +173,6 @@ void QuicSentPacketManager::OnRetransmittedPacket( previous_transmissions->insert(new_sequence_number); unacked_packets_[new_sequence_number].previous_transmissions = previous_transmissions; - - DCHECK(HasRetransmittableFrames(new_sequence_number)); } bool QuicSentPacketManager::OnIncomingAck( @@ -265,6 +263,9 @@ void QuicSentPacketManager::ClearPreviousRetransmissions(size_t num_to_clear) { if (sequence_number == newest_transmission) { break; } + if (it->second.pending) { + break; + } DCHECK(it->second.retransmittable_frames == NULL); previous_transmissions->erase(sequence_number); @@ -272,7 +273,6 @@ void QuicSentPacketManager::ClearPreviousRetransmissions(size_t num_to_clear) { unacked_packets_[newest_transmission].previous_transmissions = NULL; delete previous_transmissions; } - DCHECK(!it->second.pending); unacked_packets_.erase(it++); --num_to_clear; } @@ -314,8 +314,14 @@ void QuicSentPacketManager::RetransmitUnackedPackets( void QuicSentPacketManager::MarkForRetransmission( QuicPacketSequenceNumber sequence_number, TransmissionType transmission_type) { - DCHECK(ContainsKey(unacked_packets_, sequence_number)); - DCHECK(HasRetransmittableFrames(sequence_number)); + TransmissionInfo* transmission_info = + FindOrNull(unacked_packets_, sequence_number); + if (transmission_info != NULL) { + LOG_IF(DFATAL, transmission_info->retransmittable_frames == NULL); + LOG_IF(DFATAL, transmission_info->sent_time == QuicTime::Zero()); + } else { + LOG(DFATAL) << "Unable to retansmit packet: " << sequence_number; + } // TODO(ianswett): Currently the RTO can fire while there are pending NACK // retransmissions for the same data, which is not ideal. if (ContainsKey(pending_retransmissions_, sequence_number)) { @@ -345,26 +351,6 @@ QuicSentPacketManager::PendingRetransmission transmission_info.sequence_number_length); } -bool QuicSentPacketManager::IsPreviousTransmission( - QuicPacketSequenceNumber sequence_number) const { - DCHECK(ContainsKey(unacked_packets_, sequence_number)); - - UnackedPacketMap::const_iterator unacked_it = - unacked_packets_.find(sequence_number); - if (unacked_it == unacked_packets_.end()) { - return false; - } - const TransmissionInfo* transmission_info = &unacked_it->second; - if (transmission_info->previous_transmissions == NULL) { - return false; - } - - SequenceNumberSet* previous_transmissions = - transmission_info->previous_transmissions; - DCHECK(!previous_transmissions->empty()); - return *previous_transmissions->rbegin() != sequence_number; -} - // static bool QuicSentPacketManager::HasCryptoHandshake( const TransmissionInfo& transmission_info) { @@ -508,8 +494,7 @@ size_t QuicSentPacketManager::GetNumRetransmittablePackets() const { size_t num_unacked_packets = 0; for (UnackedPacketMap::const_iterator it = unacked_packets_.begin(); it != unacked_packets_.end(); ++it) { - QuicPacketSequenceNumber sequence_number = it->first; - if (HasRetransmittableFrames(sequence_number)) { + if (it->second.retransmittable_frames != NULL) { ++num_unacked_packets; } } diff --git a/net/quic/quic_sent_packet_manager.h b/net/quic/quic_sent_packet_manager.h index 8339fba..f608c5f 100644 --- a/net/quic/quic_sent_packet_manager.h +++ b/net/quic/quic_sent_packet_manager.h @@ -130,9 +130,6 @@ class NET_EXPORT_PRIVATE QuicSentPacketManager { // Test only. SequenceNumberSet GetUnackedPackets() const; - // Returns true if |sequence_number| is a previous transmission of packet. - bool IsPreviousTransmission(QuicPacketSequenceNumber sequence_number) const; - // Called when a congestion feedback frame is received from peer. virtual void OnIncomingQuicCongestionFeedbackFrame( const QuicCongestionFeedbackFrame& frame, diff --git a/net/quic/quic_sent_packet_manager_test.cc b/net/quic/quic_sent_packet_manager_test.cc index 832b8b9..ee6f949 100644 --- a/net/quic/quic_sent_packet_manager_test.cc +++ b/net/quic/quic_sent_packet_manager_test.cc @@ -191,9 +191,7 @@ TEST_F(QuicSentPacketManagerTest, IsUnacked) { } TEST_F(QuicSentPacketManagerTest, IsUnAckedRetransmit) { - SerializedPacket serialized_packet(CreateDataPacket(1)); - - manager_.OnSerializedPacket(serialized_packet); + SendDataPacket(1); RetransmitPacket(1, 2); EXPECT_TRUE(QuicSentPacketManagerPeer::IsRetransmission(&manager_, 2)); @@ -204,26 +202,26 @@ TEST_F(QuicSentPacketManagerTest, IsUnAckedRetransmit) { } TEST_F(QuicSentPacketManagerTest, RetransmitThenAck) { - SerializedPacket serialized_packet(CreateDataPacket(1)); - - manager_.OnSerializedPacket(serialized_packet); - RetransmitPacket(1, 2); + SendDataPacket(1); + RetransmitAndSendPacket(1, 2); // Ack 2 but not 1. ReceivedPacketInfo received_info; received_info.largest_observed = 2; received_info.missing_packets.insert(1); + EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); + EXPECT_CALL(*send_algorithm_, OnPacketAcked(2, _)).Times(1); manager_.OnIncomingAck(received_info, QuicTime::Zero()); - // No unacked packets remain. - VerifyUnackedPackets(NULL, 0); + // Packet 1 is unacked, pending, but not retransmittable. + QuicPacketSequenceNumber unacked[] = { 1 }; + VerifyUnackedPackets(unacked, arraysize(unacked)); + EXPECT_TRUE(QuicSentPacketManagerPeer::HasPendingPackets(&manager_)); VerifyRetransmittablePackets(NULL, 0); } TEST_F(QuicSentPacketManagerTest, RetransmitThenAckBeforeSend) { - SerializedPacket serialized_packet(CreateDataPacket(1)); - - manager_.OnSerializedPacket(serialized_packet); + SendDataPacket(1); QuicSentPacketManagerPeer::MarkForRetransmission( &manager_, 1, NACK_RETRANSMISSION); EXPECT_TRUE(manager_.HasPendingRetransmissions()); @@ -231,6 +229,8 @@ TEST_F(QuicSentPacketManagerTest, RetransmitThenAckBeforeSend) { // Ack 1. ReceivedPacketInfo received_info; received_info.largest_observed = 1; + EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); + EXPECT_CALL(*send_algorithm_, OnPacketAcked(1, _)).Times(1); manager_.OnIncomingAck(received_info, QuicTime::Zero()); // There should no longer be a pending retransmission. @@ -389,19 +389,23 @@ TEST_F(QuicSentPacketManagerTest, RetransmitTwiceThenAckFirst) { } TEST_F(QuicSentPacketManagerTest, TruncatedAck) { - SerializedPacket serialized_packet(CreateDataPacket(1)); - - manager_.OnSerializedPacket(serialized_packet); - RetransmitPacket(1, 2); - RetransmitPacket(2, 3); - RetransmitPacket(3, 4); + SendDataPacket(1); + RetransmitAndSendPacket(1, 2); + RetransmitAndSendPacket(2, 3); + RetransmitAndSendPacket(3, 4); + RetransmitAndSendPacket(4, 5); - // Truncated ack with 2 NACKs + // Truncated ack with 4 NACKs, so the first packet is lost. ReceivedPacketInfo received_info; - received_info.largest_observed = 2; + received_info.largest_observed = 4; received_info.missing_packets.insert(1); received_info.missing_packets.insert(2); + received_info.missing_packets.insert(3); + received_info.missing_packets.insert(4); received_info.is_truncated = true; + EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); + EXPECT_CALL(*send_algorithm_, OnPacketLost(1, _)); + EXPECT_CALL(*send_algorithm_, OnPacketAbandoned(1, _)); manager_.OnIncomingAck(received_info, QuicTime::Zero()); // High water mark will be raised. @@ -412,12 +416,10 @@ TEST_F(QuicSentPacketManagerTest, TruncatedAck) { } TEST_F(QuicSentPacketManagerTest, AckPreviousTransmissionThenTruncatedAck) { - SerializedPacket serialized_packet(CreateDataPacket(1)); - - manager_.OnSerializedPacket(serialized_packet); - RetransmitPacket(1, 2); - RetransmitPacket(2, 3); - RetransmitPacket(3, 4); + SendDataPacket(1); + RetransmitAndSendPacket(1, 2); + RetransmitAndSendPacket(2, 3); + RetransmitAndSendPacket(3, 4); manager_.OnSerializedPacket(CreateDataPacket(5)); manager_.OnSerializedPacket(CreateDataPacket(6)); manager_.OnSerializedPacket(CreateDataPacket(7)); @@ -429,6 +431,8 @@ TEST_F(QuicSentPacketManagerTest, AckPreviousTransmissionThenTruncatedAck) { ReceivedPacketInfo received_info; received_info.largest_observed = 2; received_info.missing_packets.insert(1); + EXPECT_CALL(*send_algorithm_, UpdateRtt(_)); + EXPECT_CALL(*send_algorithm_, OnPacketAcked(2, _)); manager_.OnIncomingAck(received_info, QuicTime::Zero()); EXPECT_TRUE(manager_.IsUnacked(4)); } @@ -442,6 +446,9 @@ TEST_F(QuicSentPacketManagerTest, AckPreviousTransmissionThenTruncatedAck) { received_info.missing_packets.insert(5); received_info.missing_packets.insert(6); received_info.is_truncated = true; + EXPECT_CALL(*send_algorithm_, OnPacketAcked(1, _)); + EXPECT_CALL(*send_algorithm_, OnPacketAbandoned(3, _)); + EXPECT_CALL(*send_algorithm_, OnPacketAbandoned(4, _)); manager_.OnIncomingAck(received_info, QuicTime::Zero()); } @@ -452,130 +459,6 @@ TEST_F(QuicSentPacketManagerTest, AckPreviousTransmissionThenTruncatedAck) { VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); } -TEST_F(QuicSentPacketManagerTest, SendDropAckRetransmitManyPackets) { - manager_.OnSerializedPacket(CreateDataPacket(1)); - manager_.OnSerializedPacket(CreateDataPacket(2)); - manager_.OnSerializedPacket(CreateDataPacket(3)); - - { - // Ack packets 1 and 3. - ReceivedPacketInfo received_info; - received_info.largest_observed = 3; - received_info.missing_packets.insert(2); - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - QuicPacketSequenceNumber unacked[] = { 2 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 2 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } - - manager_.OnSerializedPacket(CreateDataPacket(4)); - manager_.OnSerializedPacket(CreateDataPacket(5)); - - { - // Ack packets 5. - ReceivedPacketInfo received_info; - received_info.largest_observed = 5; - received_info.missing_packets.insert(2); - received_info.missing_packets.insert(4); - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - QuicPacketSequenceNumber unacked[] = { 2, 4 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 2, 4 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } - - manager_.OnSerializedPacket(CreateDataPacket(6)); - manager_.OnSerializedPacket(CreateDataPacket(7)); - - { - // Ack packets 7. - ReceivedPacketInfo received_info; - received_info.largest_observed = 7; - received_info.missing_packets.insert(2); - received_info.missing_packets.insert(4); - received_info.missing_packets.insert(6); - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - QuicPacketSequenceNumber unacked[] = { 2, 4, 6 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 2, 4, 6 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } - - RetransmitPacket(2, 8); - manager_.OnSerializedPacket(CreateDataPacket(9)); - manager_.OnSerializedPacket(CreateDataPacket(10)); - - { - // Ack packet 10. - ReceivedPacketInfo received_info; - received_info.largest_observed = 10; - received_info.missing_packets.insert(2); - received_info.missing_packets.insert(4); - received_info.missing_packets.insert(6); - received_info.missing_packets.insert(8); - received_info.missing_packets.insert(9); - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - QuicPacketSequenceNumber unacked[] = { 2, 4, 6, 8, 9 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 4, 6, 8, 9 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } - - - RetransmitPacket(4, 11); - manager_.OnSerializedPacket(CreateDataPacket(12)); - manager_.OnSerializedPacket(CreateDataPacket(13)); - - { - // Ack packet 13. - ReceivedPacketInfo received_info; - received_info.largest_observed = 13; - received_info.missing_packets.insert(2); - received_info.missing_packets.insert(4); - received_info.missing_packets.insert(6); - received_info.missing_packets.insert(8); - received_info.missing_packets.insert(9); - received_info.missing_packets.insert(11); - received_info.missing_packets.insert(12); - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - QuicPacketSequenceNumber unacked[] = { 2, 4, 6, 8, 9, 11, 12 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 6, 8, 9, 11, 12 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } - - RetransmitPacket(6, 14); - manager_.OnSerializedPacket(CreateDataPacket(15)); - manager_.OnSerializedPacket(CreateDataPacket(16)); - - { - // Ack packet 16. - ReceivedPacketInfo received_info; - received_info.largest_observed = 13; - received_info.missing_packets.insert(2); - received_info.missing_packets.insert(4); - received_info.missing_packets.insert(6); - received_info.missing_packets.insert(8); - received_info.missing_packets.insert(9); - received_info.missing_packets.insert(11); - received_info.missing_packets.insert(12); - received_info.is_truncated = true; - manager_.OnIncomingAck(received_info, QuicTime::Zero()); - - // Truncated ack raises the high water mark by clearing out 2, 4, and 6. - QuicPacketSequenceNumber unacked[] = { 8, 9, 11, 12, 14, 15, 16 }; - VerifyUnackedPackets(unacked, arraysize(unacked)); - QuicPacketSequenceNumber retransmittable[] = { 8, 9, 11, 12, 14, 15, 16 }; - VerifyRetransmittablePackets(retransmittable, arraysize(retransmittable)); - } -} - TEST_F(QuicSentPacketManagerTest, GetLeastUnackedSentPacket) { EXPECT_CALL(helper_, GetNextPacketSequenceNumber()).WillOnce(Return(1u)); EXPECT_EQ(1u, manager_.GetLeastUnackedSentPacket()); diff --git a/net/quic/test_tools/quic_test_packet_maker.cc b/net/quic/test_tools/quic_test_packet_maker.cc index 50303b8..38bd0c0 100644 --- a/net/quic/test_tools/quic_test_packet_maker.cc +++ b/net/quic/test_tools/quic_test_packet_maker.cc @@ -40,6 +40,47 @@ scoped_ptr QuicTestPacketMaker::MakeRstPacket( return scoped_ptr(MakePacket(header, QuicFrame(&rst))); } +scoped_ptr QuicTestPacketMaker::MakeAckAndRstPacket( + QuicPacketSequenceNumber num, + bool include_version, + QuicStreamId stream_id, + QuicRstStreamErrorCode error_code, + QuicPacketSequenceNumber largest_received, + QuicPacketSequenceNumber least_unacked, + bool send_feedback) { + + QuicPacketHeader header; + header.public_header.guid = guid_; + header.public_header.reset_flag = false; + header.public_header.version_flag = include_version; + header.public_header.sequence_number_length = PACKET_1BYTE_SEQUENCE_NUMBER; + header.packet_sequence_number = num; + header.entropy_flag = false; + header.fec_flag = false; + header.fec_group = 0; + + QuicAckFrame ack(largest_received, QuicTime::Zero(), least_unacked); + QuicFrames frames; + frames.push_back(QuicFrame(&ack)); + if (send_feedback) { + QuicCongestionFeedbackFrame feedback; + feedback.type = kTCP; + feedback.tcp.accumulated_number_of_lost_packets = 0; + feedback.tcp.receive_window = 256000; + + frames.push_back(QuicFrame(&feedback)); + } + + QuicRstStreamFrame rst(stream_id, error_code); + frames.push_back(QuicFrame(&rst)); + + QuicFramer framer(SupportedVersions(version_), QuicTime::Zero(), false); + scoped_ptr packet( + framer.BuildUnsizedDataPacket(header, frames).packet); + return scoped_ptr(framer.EncryptPacket( + ENCRYPTION_NONE, header.packet_sequence_number, *packet)); +} + scoped_ptr QuicTestPacketMaker::MakeConnectionClosePacket( QuicPacketSequenceNumber num) { QuicPacketHeader header; diff --git a/net/quic/test_tools/quic_test_packet_maker.h b/net/quic/test_tools/quic_test_packet_maker.h index ac5051c..47d606e 100644 --- a/net/quic/test_tools/quic_test_packet_maker.h +++ b/net/quic/test_tools/quic_test_packet_maker.h @@ -28,6 +28,14 @@ class QuicTestPacketMaker { bool include_version, QuicStreamId stream_id, QuicRstStreamErrorCode error_code); + scoped_ptr MakeAckAndRstPacket( + QuicPacketSequenceNumber num, + bool include_version, + QuicStreamId stream_id, + QuicRstStreamErrorCode error_code, + QuicPacketSequenceNumber largest_received, + QuicPacketSequenceNumber least_unacked, + bool send_feedback); scoped_ptr MakeConnectionClosePacket( QuicPacketSequenceNumber num); scoped_ptr MakeAckPacket( diff --git a/net/quic/test_tools/quic_test_utils.cc b/net/quic/test_tools/quic_test_utils.cc index 78ffa35..17d0ac5 100644 --- a/net/quic/test_tools/quic_test_utils.cc +++ b/net/quic/test_tools/quic_test_utils.cc @@ -307,8 +307,8 @@ bool PacketSavingConnection::SendOrQueuePacket( const SerializedPacket& packet, TransmissionType transmission_type) { packets_.push_back(packet.packet); - QuicEncryptedPacket* encrypted = - framer_.EncryptPacket(level, packet.sequence_number, *packet.packet); + QuicEncryptedPacket* encrypted = QuicConnectionPeer::GetFramer(this)-> + EncryptPacket(level, packet.sequence_number, *packet.packet); encrypted_packets_.push_back(encrypted); return true; } diff --git a/net/quic/test_tools/quic_test_utils.h b/net/quic/test_tools/quic_test_utils.h index 4c5be0d..a77cb08 100644 --- a/net/quic/test_tools/quic_test_utils.h +++ b/net/quic/test_tools/quic_test_utils.h @@ -385,12 +385,11 @@ class MockPacketWriter : public QuicPacketWriter { MockPacketWriter(); virtual ~MockPacketWriter(); - MOCK_METHOD5(WritePacket, + MOCK_METHOD4(WritePacket, WriteResult(const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer)); + const IPEndPoint& peer_address)); MOCK_CONST_METHOD0(IsWriteBlockedDataBuffered, bool()); MOCK_CONST_METHOD0(IsWriteBlocked, bool()); MOCK_METHOD0(SetWritable, void()); diff --git a/net/tools/quic/end_to_end_test.cc b/net/tools/quic/end_to_end_test.cc index 3e22818..86cd818 100644 --- a/net/tools/quic/end_to_end_test.cc +++ b/net/tools/quic/end_to_end_test.cc @@ -4,6 +4,7 @@ #include #include +#include #include #include "base/basictypes.h" @@ -146,6 +147,28 @@ vector GetTestParams() { return params; } +class ServerDelegate : public PacketDroppingTestWriter::Delegate { + public: + explicit ServerDelegate(QuicDispatcher* dispatcher) + : dispatcher_(dispatcher) {} + virtual ~ServerDelegate() {} + virtual void OnCanWrite() OVERRIDE { dispatcher_->OnCanWrite(); } + private: + QuicDispatcher* dispatcher_; +}; + +class ClientDelegate : public PacketDroppingTestWriter::Delegate { + public: + explicit ClientDelegate(QuicClient* client) : client_(client) {} + virtual ~ClientDelegate() {} + virtual void OnCanWrite() OVERRIDE { + EpollEvent event(EPOLLOUT, false); + client_->OnEvent(client_->fd(), &event); + } + private: + QuicClient* client_; +}; + class EndToEndTest : public ::testing::TestWithParam { protected: EndToEndTest() @@ -180,7 +203,7 @@ class EndToEndTest : public ::testing::TestWithParam { QuicInMemoryCachePeer::ResetForTests(); } - virtual QuicTestClient* CreateQuicClient(QuicPacketWriterWrapper* writer) { + QuicTestClient* CreateQuicClient(QuicPacketWriterWrapper* writer) { QuicTestClient* client = new QuicTestClient(server_address_, server_hostname_, false, // not secure @@ -191,27 +214,28 @@ class EndToEndTest : public ::testing::TestWithParam { return client; } - virtual bool Initialize() { + bool Initialize() { // Start the server first, because CreateQuicClient() attempts // to connect to the server. StartServer(); client_.reset(CreateQuicClient(client_writer_)); - QuicEpollConnectionHelper* helper = + static EpollEvent event(EPOLLOUT, false); + client_writer_->Initialize( reinterpret_cast( QuicConnectionPeer::GetHelper( - client_->client()->session()->connection())); - client_writer_->SetConnectionHelper(helper); + client_->client()->session()->connection())), + new ClientDelegate(client_->client())); return client_->client()->connected(); } - virtual void SetUp() { + virtual void SetUp() OVERRIDE { // The ownership of these gets transferred to the QuicPacketWriterWrapper // and QuicDispatcher when Initialize() is executed. client_writer_ = new PacketDroppingTestWriter(); server_writer_ = new PacketDroppingTestWriter(); } - virtual void TearDown() { + virtual void TearDown() OVERRIDE { StopServer(); } @@ -225,8 +249,9 @@ class EndToEndTest : public ::testing::TestWithParam { QuicDispatcher* dispatcher = QuicServerPeer::GetDispatcher(server_thread_->server()); QuicDispatcherPeer::UseWriter(dispatcher, server_writer_); - server_writer_->SetConnectionHelper( - QuicDispatcherPeer::GetHelper(dispatcher)); + server_writer_->Initialize( + QuicDispatcherPeer::GetHelper(dispatcher), + new ServerDelegate(dispatcher)); server_thread_->Start(); server_started_ = true; } @@ -466,8 +491,7 @@ TEST_P(EndToEndTest, LargePostNoPacketLossWithDelayAndReordering) { EXPECT_EQ(kFooResponseBody, client_->SendCustomSynchronousRequest(request)); } -// TODO(ianswett): Re-enable once b/12646613 and b/11206052 are fixed. -TEST_P(EndToEndTest, DISABLED_LargePostWithPacketLossAndBlockedSocket) { +TEST_P(EndToEndTest, LargePostWithPacketLossAndBlockedSocket) { // Connect with lower fake packet loss than we'd like to test. Until // b/10126687 is fixed, losing handshake packets is pretty brutal. SetPacketLossPercentage(5); @@ -475,7 +499,7 @@ TEST_P(EndToEndTest, DISABLED_LargePostWithPacketLossAndBlockedSocket) { // Wait for the server SHLO before upping the packet loss. client_->client()->WaitForCryptoHandshakeConfirmed(); - SetPacketLossPercentage(30); + SetPacketLossPercentage(10); client_writer_->set_fake_blocked_socket_percentage(10); // 10 Kb body. @@ -814,11 +838,10 @@ class WrongAddressWriter : public QuicPacketWriterWrapper { const char* buffer, size_t buf_len, const IPAddressNumber& real_self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE { + const IPEndPoint& peer_address) OVERRIDE { // Use wrong address! return QuicPacketWriterWrapper::WritePacket( - buffer, buf_len, self_address_.address(), peer_address, blocked_writer); + buffer, buf_len, self_address_.address(), peer_address); } virtual bool IsWriteBlockedDataBuffered() const OVERRIDE { diff --git a/net/tools/quic/quic_client.cc b/net/tools/quic/quic_client.cc index 56648ad..cdb15f0 100644 --- a/net/tools/quic/quic_client.cc +++ b/net/tools/quic/quic_client.cc @@ -239,6 +239,7 @@ void QuicClient::OnEvent(int fd, EpollEvent* event) { } } if (connected() && (event->in_events & EPOLLOUT)) { + writer_->SetWritable(); session_->connection()->OnCanWrite(); } if (event->in_events & EPOLLERR) { diff --git a/net/tools/quic/quic_default_packet_writer.cc b/net/tools/quic/quic_default_packet_writer.cc index 6d77c00..ff83be6 100644 --- a/net/tools/quic/quic_default_packet_writer.cc +++ b/net/tools/quic/quic_default_packet_writer.cc @@ -18,8 +18,7 @@ QuicDefaultPacketWriter::~QuicDefaultPacketWriter() {} WriteResult QuicDefaultPacketWriter::WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) { + const net::IPEndPoint& peer_address) { DCHECK(!IsWriteBlocked()); WriteResult result = QuicSocketUtils::WritePacket( fd_, buffer, buf_len, self_address, peer_address); diff --git a/net/tools/quic/quic_default_packet_writer.h b/net/tools/quic/quic_default_packet_writer.h index 9f21107..5e8c25f 100644 --- a/net/tools/quic/quic_default_packet_writer.h +++ b/net/tools/quic/quic_default_packet_writer.h @@ -11,7 +11,6 @@ namespace net { -class QuicBlockedWriterInterface; struct WriteResult; namespace tools { @@ -26,8 +25,7 @@ class QuicDefaultPacketWriter : public QuicPacketWriter { virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE; + const net::IPEndPoint& peer_address) OVERRIDE; virtual bool IsWriteBlockedDataBuffered() const OVERRIDE; virtual bool IsWriteBlocked() const OVERRIDE; virtual void SetWritable() OVERRIDE; diff --git a/net/tools/quic/quic_dispatcher.cc b/net/tools/quic/quic_dispatcher.cc index e5fecc9..4604209 100644 --- a/net/tools/quic/quic_dispatcher.cc +++ b/net/tools/quic/quic_dispatcher.cc @@ -150,38 +150,6 @@ void QuicDispatcher::Initialize(int fd) { epoll_server(), supported_versions())); } -// TODO(fnk): remove the Writer interface implementation in favor of -// direct requests for blocked list placement from Connection/Session. -WriteResult QuicDispatcher::WritePacket(const char* buffer, size_t buf_len, - const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* writer) { - if (IsWriteBlocked()) { - write_blocked_list_.insert(make_pair(writer, true)); - return WriteResult(WRITE_STATUS_BLOCKED, EAGAIN); - } - - WriteResult result = - writer_->WritePacket(buffer, buf_len, self_address, peer_address, writer); - if (result.status == WRITE_STATUS_BLOCKED) { - DCHECK(IsWriteBlocked()); - write_blocked_list_.insert(make_pair(writer, true)); - } - return result; -} - -bool QuicDispatcher::IsWriteBlockedDataBuffered() const { - return writer_->IsWriteBlockedDataBuffered(); -} - -bool QuicDispatcher::IsWriteBlocked() const { - return writer_->IsWriteBlocked(); -} - -void QuicDispatcher::SetWritable() { - writer_->SetWritable(); -} - void QuicDispatcher::ProcessPacket(const IPEndPoint& server_address, const IPEndPoint& client_address, const QuicEncryptedPacket& packet) { @@ -273,7 +241,7 @@ void QuicDispatcher::DeleteSessions() { bool QuicDispatcher::OnCanWrite() { // We got an EPOLLOUT: the socket should not be blocked. - SetWritable(); + writer_->SetWritable(); // Give each writer one attempt to write. int num_writers = write_blocked_list_.size(); @@ -284,7 +252,7 @@ bool QuicDispatcher::OnCanWrite() { QuicBlockedWriterInterface* writer = write_blocked_list_.begin()->first; write_blocked_list_.erase(write_blocked_list_.begin()); bool can_write_more = writer->OnCanWrite(); - if (IsWriteBlocked()) { + if (writer_->IsWriteBlocked()) { // We were unable to write. Wait for the next EPOLLOUT. // In this case, the session would have been added to the blocked list // up in WritePacket. @@ -336,24 +304,33 @@ void QuicDispatcher::OnWriteBlocked(QuicBlockedWriterInterface* writer) { write_blocked_list_.insert(make_pair(writer, true)); } +QuicPacketWriter* QuicDispatcher::CreateWriter(int fd) { + return new QuicDefaultPacketWriter(fd); +} + +QuicPacketWriterWrapper* QuicDispatcher::CreateWriterWrapper( + QuicPacketWriter* writer) { + return new QuicPacketWriterWrapper(writer); +} + QuicSession* QuicDispatcher::CreateQuicSession( QuicGuid guid, const IPEndPoint& server_address, const IPEndPoint& client_address) { QuicServerSession* session = new QuicServerSession( - config_, new QuicConnection(guid, client_address, helper_.get(), this, - true, supported_versions_), this); + config_, + CreateQuicConnection(guid, server_address, client_address), + this); session->InitializeSession(crypto_config_); return session; } -QuicPacketWriter* QuicDispatcher::CreateWriter(int fd) { - return new QuicDefaultPacketWriter(fd); -} - -QuicPacketWriterWrapper* QuicDispatcher::CreateWriterWrapper( - QuicPacketWriter* writer) { - return new QuicPacketWriterWrapper(writer); +QuicConnection* QuicDispatcher::CreateQuicConnection( + QuicGuid guid, + const IPEndPoint& server_address, + const IPEndPoint& client_address) { + return new QuicConnection(guid, client_address, helper_.get(), writer_.get(), + true, supported_versions_); } void QuicDispatcher::set_writer(QuicPacketWriter* writer) { diff --git a/net/tools/quic/quic_dispatcher.h b/net/tools/quic/quic_dispatcher.h index 47c31d2..cc72618 100644 --- a/net/tools/quic/quic_dispatcher.h +++ b/net/tools/quic/quic_dispatcher.h @@ -16,7 +16,6 @@ #include "net/base/ip_endpoint.h" #include "net/base/linked_hash_map.h" #include "net/quic/quic_blocked_writer_interface.h" -#include "net/quic/quic_packet_writer.h" #include "net/quic/quic_protocol.h" #include "net/tools/epoll_server/epoll_server.h" #include "net/tools/quic/quic_server_session.h" @@ -53,8 +52,7 @@ class QuicDispatcherPeer; class DeleteSessionsAlarm; class QuicEpollConnectionHelper; -class QuicDispatcher : public QuicPacketWriter, - public QuicServerSessionVisitor { +class QuicDispatcher : public QuicServerSessionVisitor { public: // Ideally we'd have a linked_hash_set: the boolean is unused. typedef linked_hash_map WriteBlockedList; @@ -71,16 +69,6 @@ class QuicDispatcher : public QuicPacketWriter, void Initialize(int fd); - // QuicPacketWriter - virtual WriteResult WritePacket( - const char* buffer, size_t buf_len, - const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* writer) OVERRIDE; - virtual bool IsWriteBlockedDataBuffered() const OVERRIDE; - virtual bool IsWriteBlocked() const OVERRIDE; - virtual void SetWritable() OVERRIDE; - // Process the incoming packet by creating a new session, passing it to // an existing session, or passing it to the TimeWaitListManager. virtual void ProcessPacket(const IPEndPoint& server_address, @@ -105,11 +93,6 @@ class QuicDispatcher : public QuicPacketWriter, typedef base::hash_map SessionMap; - virtual QuicSession* CreateQuicSession( - QuicGuid guid, - const IPEndPoint& server_address, - const IPEndPoint& client_address); - // Deletes all sessions on the closed session list and clears the list. void DeleteSessions(); @@ -127,6 +110,14 @@ class QuicDispatcher : public QuicPacketWriter, virtual QuicPacketWriterWrapper* CreateWriterWrapper( QuicPacketWriter* writer); + virtual QuicSession* CreateQuicSession(QuicGuid guid, + const IPEndPoint& server_address, + const IPEndPoint& client_address); + + QuicConnection* CreateQuicConnection(QuicGuid guid, + const IPEndPoint& server_address, + const IPEndPoint& client_address); + // Replaces the packet writer with |writer|. Takes ownership of |writer|. void set_writer(QuicPacketWriter* writer); diff --git a/net/tools/quic/quic_dispatcher_test.cc b/net/tools/quic/quic_dispatcher_test.cc index 5e4726f..4434b5a 100644 --- a/net/tools/quic/quic_dispatcher_test.cc +++ b/net/tools/quic/quic_dispatcher_test.cc @@ -284,13 +284,12 @@ class BlockingWriter : public QuicPacketWriterWrapper { const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE { + const IPEndPoint& peer_address) OVERRIDE { if (write_blocked_) { return WriteResult(WRITE_STATUS_BLOCKED, EAGAIN); } else { return QuicPacketWriterWrapper::WritePacket( - buffer, buf_len, self_address, peer_address, blocked_writer); + buffer, buf_len, self_address, peer_address); } } diff --git a/net/tools/quic/quic_packet_writer_wrapper.cc b/net/tools/quic/quic_packet_writer_wrapper.cc index f45b90c..e4e8a40 100644 --- a/net/tools/quic/quic_packet_writer_wrapper.cc +++ b/net/tools/quic/quic_packet_writer_wrapper.cc @@ -20,10 +20,8 @@ WriteResult QuicPacketWriterWrapper::WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) { - return writer_->WritePacket( - buffer, buf_len, self_address, peer_address, blocked_writer); + const net::IPEndPoint& peer_address) { + return writer_->WritePacket(buffer, buf_len, self_address, peer_address); } bool QuicPacketWriterWrapper::IsWriteBlockedDataBuffered() const { diff --git a/net/tools/quic/quic_packet_writer_wrapper.h b/net/tools/quic/quic_packet_writer_wrapper.h index 4a74ce8..9dafe77 100644 --- a/net/tools/quic/quic_packet_writer_wrapper.h +++ b/net/tools/quic/quic_packet_writer_wrapper.h @@ -27,8 +27,7 @@ class QuicPacketWriterWrapper : public net::QuicPacketWriter { const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE; + const IPEndPoint& peer_address) OVERRIDE; virtual bool IsWriteBlockedDataBuffered() const OVERRIDE; virtual bool IsWriteBlocked() const OVERRIDE; virtual void SetWritable() OVERRIDE; diff --git a/net/tools/quic/quic_time_wait_list_manager.cc b/net/tools/quic/quic_time_wait_list_manager.cc index 53feec1..2f4eb66 100644 --- a/net/tools/quic/quic_time_wait_list_manager.cc +++ b/net/tools/quic/quic_time_wait_list_manager.cc @@ -216,8 +216,7 @@ bool QuicTimeWaitListManager::WriteToWire(QueuedPacket* queued_packet) { queued_packet->packet()->data(), queued_packet->packet()->length(), queued_packet->server_address().address(), - queued_packet->client_address(), - this); + queued_packet->client_address()); if (result.status == WRITE_STATUS_BLOCKED) { // If blocked and unbuffered, return false to retry sending. DCHECK(writer_->IsWriteBlocked()); 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 0acaaea..2637127 100644 --- a/net/tools/quic/quic_time_wait_list_manager_test.cc +++ b/net/tools/quic/quic_time_wait_list_manager_test.cc @@ -200,8 +200,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendConnectionClose) { const int kRandomSequenceNumber = 1; EXPECT_CALL(writer_, WritePacket(_, kConnectionCloseLength, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 1))); ProcessPacket(guid_, kRandomSequenceNumber); @@ -212,8 +211,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendPublicReset) { const int kRandomSequenceNumber = 1; EXPECT_CALL(writer_, WritePacket(_, _, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .With(Args<0, 1>(PublicResetPacketEq(guid_, kRandomSequenceNumber))) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); @@ -225,7 +223,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendPublicResetWithExponentialBackOff) { AddGuid(guid_); for (int sequence_number = 1; sequence_number < 101; ++sequence_number) { if ((sequence_number & (sequence_number - 1)) == 0) { - EXPECT_CALL(writer_, WritePacket(_, _, _, _, _)) + EXPECT_CALL(writer_, WritePacket(_, _, _, _)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 1))); } ProcessPacket(guid_, sequence_number); @@ -283,8 +281,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { // Let first write through. EXPECT_CALL(writer_, WritePacket(_, _, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .With(Args<0, 1>(PublicResetPacketEq(guid, sequence_number))) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, packet->length()))); @@ -293,8 +290,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { // write block for the next packet. EXPECT_CALL(writer_, WritePacket(_, _, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .With(Args<0, 1>(PublicResetPacketEq(guid, sequence_number))) .WillOnce(DoAll( @@ -313,7 +309,7 @@ TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { scoped_ptr other_packet( ConstructEncryptedPacket( ENCRYPTION_NONE, other_guid, other_sequence_number)); - EXPECT_CALL(writer_, WritePacket(_, _, _, _, _)) + EXPECT_CALL(writer_, WritePacket(_, _, _, _)) .Times(0); EXPECT_CALL(visitor_, OnWriteBlocked(&time_wait_list_manager_)); ProcessPacket(other_guid, other_sequence_number); @@ -322,15 +318,13 @@ TEST_F(QuicTimeWaitListManagerTest, SendQueuedPackets) { writer_is_blocked_ = false; EXPECT_CALL(writer_, WritePacket(_, _, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .With(Args<0, 1>(PublicResetPacketEq(guid, sequence_number))) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, packet->length()))); EXPECT_CALL(writer_, WritePacket(_, _, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .With(Args<0, 1>(PublicResetPacketEq(other_guid, other_sequence_number))) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, @@ -373,8 +367,7 @@ TEST_F(QuicTimeWaitListManagerTest, AddGuidTwice) { EXPECT_CALL(writer_, WritePacket(_, kConnectionCloseLength, server_address_.address(), - client_address_, - &time_wait_list_manager_)) + client_address_)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 1))); const int kRandomSequenceNumber = 1; diff --git a/net/tools/quic/test_tools/packet_dropping_test_writer.cc b/net/tools/quic/test_tools/packet_dropping_test_writer.cc index b3b7ca2..148591d 100644 --- a/net/tools/quic/test_tools/packet_dropping_test_writer.cc +++ b/net/tools/quic/test_tools/packet_dropping_test_writer.cc @@ -22,9 +22,8 @@ class WriteUnblockedAlarm : public QuicAlarm::Delegate { : writer_(writer) { } virtual QuicTime OnAlarm() OVERRIDE { - DCHECK(writer_->blocked_writer()); DVLOG(1) << "Unblocking socket."; - writer_->blocked_writer()->OnCanWrite(); + writer_->OnCanWrite(); return QuicTime::Zero(); } @@ -49,7 +48,6 @@ class DelayAlarm : public QuicAlarm::Delegate { PacketDroppingTestWriter::PacketDroppingTestWriter() : clock_(NULL), - blocked_writer_(NULL), cur_buffer_size_(0), config_mutex_(), fake_packet_loss_percentage_(0), @@ -65,21 +63,22 @@ PacketDroppingTestWriter::PacketDroppingTestWriter() PacketDroppingTestWriter::~PacketDroppingTestWriter() {} -void PacketDroppingTestWriter::SetConnectionHelper( - QuicEpollConnectionHelper* helper) { +void PacketDroppingTestWriter::Initialize( + QuicEpollConnectionHelper* helper, + Delegate* on_can_write) { clock_ = helper->GetClock(); write_unblocked_alarm_.reset( helper->CreateAlarm(new WriteUnblockedAlarm(this))); delay_alarm_.reset( helper->CreateAlarm(new DelayAlarm(this))); + on_can_write_.reset(on_can_write); } WriteResult PacketDroppingTestWriter::WritePacket( const char* buffer, size_t buf_len, const net::IPAddressNumber& self_address, - const net::IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) { + const net::IPEndPoint& peer_address) { ReleaseOldPackets(); base::AutoLock locked(config_mutex_); @@ -92,9 +91,9 @@ WriteResult PacketDroppingTestWriter::WritePacket( if (fake_blocked_socket_percentage_ > 0 && simple_random_.RandUint64() % 100 < static_cast(fake_blocked_socket_percentage_)) { + CHECK(on_can_write_.get() != NULL); DVLOG(1) << "Blocking socket."; if (!write_unblocked_alarm_->IsSet()) { - blocked_writer_ = blocked_writer; // Set the alarm to fire immediately. write_unblocked_alarm_->Set(clock_->ApproximateNow()); } @@ -132,7 +131,7 @@ WriteResult PacketDroppingTestWriter::WritePacket( } return QuicPacketWriterWrapper::WritePacket( - buffer, buf_len, self_address, peer_address, blocked_writer); + buffer, buf_len, self_address, peer_address); } bool PacketDroppingTestWriter::IsWriteBlocked() const { @@ -170,7 +169,7 @@ QuicTime PacketDroppingTestWriter::ReleaseNextPacket() { // Grab the next one off the queue and send it. QuicPacketWriterWrapper::WritePacket( iter->buffer.data(), iter->buffer.length(), - iter->self_address, iter->peer_address, NULL); + iter->self_address, iter->peer_address); DCHECK_GE(cur_buffer_size_, iter->buffer.length()); cur_buffer_size_ -= iter->buffer.length(); delayed_packets_.erase(iter); @@ -193,6 +192,10 @@ QuicTime PacketDroppingTestWriter::ReleaseOldPackets() { return QuicTime::Zero(); } +void PacketDroppingTestWriter::OnCanWrite() { + on_can_write_->OnCanWrite(); +} + PacketDroppingTestWriter::DelayedWrite::DelayedWrite( const char* buffer, size_t buf_len, diff --git a/net/tools/quic/test_tools/packet_dropping_test_writer.h b/net/tools/quic/test_tools/packet_dropping_test_writer.h index cde5c13..78f0a40 100644 --- a/net/tools/quic/test_tools/packet_dropping_test_writer.h +++ b/net/tools/quic/test_tools/packet_dropping_test_writer.h @@ -12,7 +12,6 @@ #include "base/memory/scoped_ptr.h" #include "base/synchronization/lock.h" #include "net/quic/quic_alarm.h" -#include "net/quic/quic_blocked_writer_interface.h" #include "net/tools/quic/quic_epoll_clock.h" #include "net/tools/quic/quic_packet_writer_wrapper.h" #include "net/tools/quic/test_tools/quic_test_client.h" @@ -27,19 +26,28 @@ namespace test { // the options to delay packets and reorder packets if delay is enabled. class PacketDroppingTestWriter : public QuicPacketWriterWrapper { public: + class Delegate { + public: + virtual ~Delegate() {} + virtual void OnCanWrite() = 0; + }; + PacketDroppingTestWriter(); virtual ~PacketDroppingTestWriter(); - void SetConnectionHelper(QuicEpollConnectionHelper* helper); + // Must be called before blocking, reordering or delaying (loss is OK). May be + // called after connecting if the helper is not available before. + // |on_can_write| will be triggered when fake-unblocking; ownership will be + // assumed. + void Initialize(QuicEpollConnectionHelper* helper, Delegate* on_can_write); // QuicPacketWriter methods: virtual WriteResult WritePacket( const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer) OVERRIDE; + const IPEndPoint& peer_address) OVERRIDE; virtual bool IsWriteBlocked() const OVERRIDE; @@ -50,7 +58,7 @@ class PacketDroppingTestWriter : public QuicPacketWriterWrapper { // for the next delayed packet to be written. QuicTime ReleaseOldPackets(); - QuicBlockedWriterInterface* blocked_writer() { return blocked_writer_; } + void OnCanWrite(); // The percent of time a packet is simulated as being lost. void set_fake_packet_loss_percentage(int32 fake_packet_loss_percentage) { @@ -125,7 +133,7 @@ class PacketDroppingTestWriter : public QuicPacketWriterWrapper { const QuicClock* clock_; scoped_ptr write_unblocked_alarm_; scoped_ptr delay_alarm_; - QuicBlockedWriterInterface* blocked_writer_; + scoped_ptr on_can_write_; SimpleRandom simple_random_; // Stored packets delayed by fake packet delay or bandwidth restrictions. DelayedPacketList delayed_packets_; diff --git a/net/tools/quic/test_tools/quic_test_utils.h b/net/tools/quic/test_tools/quic_test_utils.h index af46b77..1287ed0 100644 --- a/net/tools/quic/test_tools/quic_test_utils.h +++ b/net/tools/quic/test_tools/quic_test_utils.h @@ -117,12 +117,11 @@ class MockPacketWriter : public QuicPacketWriter { MockPacketWriter(); virtual ~MockPacketWriter(); - MOCK_METHOD5(WritePacket, + MOCK_METHOD4(WritePacket, WriteResult(const char* buffer, size_t buf_len, const IPAddressNumber& self_address, - const IPEndPoint& peer_address, - QuicBlockedWriterInterface* blocked_writer)); + const IPEndPoint& peer_address)); MOCK_CONST_METHOD0(IsWriteBlockedDataBuffered, bool()); MOCK_CONST_METHOD0(IsWriteBlocked, bool()); MOCK_METHOD0(SetWritable, void()); -- cgit v1.1