Remove ack bundling mode. gfe-relnote: Deprecate gfe2_reloadable_flag_quic_deprecate_ack_bundling_mode. PiperOrigin-RevId: 253856112 Change-Id: Iaa62b97be5904743c56c5b48b230446a9c4bd870
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index fa60c96..a64b289 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -3612,8 +3612,7 @@ QuicConnectionPeer::GetHelper(client_connection)->GetRandomGenerator(); QuicMemSliceStorage storage(nullptr, 0, nullptr, 0); { - QuicConnection::ScopedPacketFlusher flusher( - client_session->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(client_session->connection()); // Verify the largest message gets successfully sent. EXPECT_EQ( MessageResult(MESSAGE_STATUS_SUCCESS, 1),
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc index 7003b21..8002b0c 100644 --- a/quic/core/http/quic_send_control_stream.cc +++ b/quic/core/http/quic_send_control_stream.cc
@@ -25,8 +25,7 @@ void QuicSendControlStream::SendSettingsFrame(const SettingsFrame& settings) { DCHECK(!settings_sent_); - QuicConnection::ScopedPacketFlusher flusher( - session()->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(session()->connection()); // Send the stream type on so the peer knows about this stream. char data[sizeof(kControlStream)]; QuicDataWriter writer(QUIC_ARRAYSIZE(data), data);
diff --git a/quic/core/http/quic_spdy_client_stream.cc b/quic/core/http/quic_spdy_client_stream.cc index 2903b30..5cbb79d 100644 --- a/quic/core/http/quic_spdy_client_stream.cc +++ b/quic/core/http/quic_spdy_client_stream.cc
@@ -142,8 +142,7 @@ size_t QuicSpdyClientStream::SendRequest(SpdyHeaderBlock headers, QuicStringPiece body, bool fin) { - QuicConnection::ScopedPacketFlusher flusher( - session_->connection(), QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher flusher(session_->connection()); bool send_fin_with_headers = fin && body.empty(); size_t bytes_sent = body.size(); header_bytes_written_ =
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index bac53ce..fa43d54 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -218,8 +218,7 @@ SpdyHeaderBlock header_block, bool fin, QuicReferenceCountedPointer<QuicAckListenerInterface> ack_listener) { - QuicConnection::ScopedPacketFlusher flusher( - spdy_session_->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(spdy_session_->connection()); // Send stream type for server push stream if (VersionHasStreamType(session()->connection()->transport_version()) && type() == WRITE_UNIDIRECTIONAL && send_buffer().stream_offset() == 0) { @@ -256,8 +255,7 @@ WriteOrBufferData(data, fin, nullptr); return; } - QuicConnection::ScopedPacketFlusher flusher( - spdy_session_->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(spdy_session_->connection()); // Write frame header. std::unique_ptr<char[]> buffer; @@ -344,8 +342,7 @@ return {0, false}; } - QuicConnection::ScopedPacketFlusher flusher( - spdy_session_->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(spdy_session_->connection()); // Write frame header. struct iovec header_iov = {static_cast<void*>(buffer.get()), header_length};
diff --git a/quic/core/qpack/qpack_send_stream.cc b/quic/core/qpack/qpack_send_stream.cc index 73d62f3..620ab2b 100644 --- a/quic/core/qpack/qpack_send_stream.cc +++ b/quic/core/qpack/qpack_send_stream.cc
@@ -24,8 +24,7 @@ } void QpackSendStream::WriteStreamData(QuicStringPiece data) { - QuicConnection::ScopedPacketFlusher flusher( - session()->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(session()->connection()); if (!stream_type_sent_) { char type[sizeof(stream_type_)]; QuicDataWriter writer(QUIC_ARRAYSIZE(type), type);
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index e48480c..895227c 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -81,15 +81,12 @@ void OnAlarm() override { DCHECK(connection_->ack_frame_updated()); - QuicConnection::ScopedPacketFlusher flusher(connection_, - QuicConnection::SEND_ACK); - if (connection_->packet_generator().deprecate_ack_bundling_mode()) { - if (connection_->SupportsMultiplePacketNumberSpaces()) { - connection_->SendAllPendingAcks(); - } else { - DCHECK(!connection_->GetUpdatedAckFrame().ack_frame->packets.Empty()); - connection_->SendAck(); - } + QuicConnection::ScopedPacketFlusher flusher(connection_); + if (connection_->SupportsMultiplePacketNumberSpaces()) { + connection_->SendAllPendingAcks(); + } else { + DCHECK(!connection_->GetUpdatedAckFrame().ack_frame->packets.Empty()); + connection_->SendAck(); } } @@ -193,8 +190,7 @@ const ProcessUndecryptablePacketsAlarmDelegate&) = delete; void OnAlarm() override { - QuicConnection::ScopedPacketFlusher flusher(connection_, - QuicConnection::NO_ACK); + QuicConnection::ScopedPacketFlusher flusher(connection_); connection_->MaybeProcessUndecryptablePackets(); } @@ -263,7 +259,6 @@ close_connection_after_five_rtos_(false), received_packet_manager_(&stats_), uber_received_packet_manager_(&stats_), - ack_queued_(false), num_retransmittable_packets_received_since_last_ack_sent_(0), num_packets_received_since_last_ack_sent_(0), stop_waiting_count_(0), @@ -366,9 +361,6 @@ supported_versions.size() == 1) { QUIC_RESTART_FLAG_COUNT(quic_no_server_conn_ver_negotiation2); } - if (packet_generator_.deprecate_ack_bundling_mode()) { - QUIC_RELOADABLE_FLAG_COUNT(quic_deprecate_ack_bundling_mode); - } if (received_packet_manager_.decide_when_to_send_acks()) { QUIC_RELOADABLE_FLAG_COUNT(quic_rpm_decides_when_to_send_acks); } @@ -1659,15 +1651,11 @@ sent_packet_manager_.unacked_packets().largest_sent_largest_acked(); if (largest_sent_largest_acked.IsInitialized() && last_header_.packet_number < largest_sent_largest_acked) { - if (packet_generator_.deprecate_ack_bundling_mode()) { - MaybeSetAckAlarmTo(clock_->ApproximateNow()); - } else { - ack_queued_ = true; - } + MaybeSetAckAlarmTo(clock_->ApproximateNow()); } } - if (should_last_packet_instigate_acks_ && !ack_queued_) { + if (should_last_packet_instigate_acks_) { ++num_retransmittable_packets_received_since_last_ack_sent_; if (ack_mode_ != TCP_ACKING && last_header_.packet_number >= @@ -1677,12 +1665,8 @@ if (!unlimited_ack_decimation_ && num_retransmittable_packets_received_since_last_ack_sent_ >= kMaxRetransmittablePacketsBeforeAck) { - if (packet_generator_.deprecate_ack_bundling_mode()) { - MaybeSetAckAlarmTo(clock_->ApproximateNow()); - } else { - ack_queued_ = true; - } - } else if (ShouldSetAckAlarm()) { + MaybeSetAckAlarmTo(clock_->ApproximateNow()); + } else if (!ack_alarm_->IsSet()) { // Wait for the minimum of the ack decimation delay or the delayed ack // time before sending an ack. QuicTime::Delta ack_delay = @@ -1704,12 +1688,8 @@ // Ack with a timer or every 2 packets by default. if (num_retransmittable_packets_received_since_last_ack_sent_ >= ack_frequency_before_ack_decimation_) { - if (packet_generator_.deprecate_ack_bundling_mode()) { - MaybeSetAckAlarmTo(clock_->ApproximateNow()); - } else { - ack_queued_ = true; - } - } else if (ShouldSetAckAlarm()) { + MaybeSetAckAlarmTo(clock_->ApproximateNow()); + } else if (!ack_alarm_->IsSet()) { const QuicTime approximate_now = clock_->ApproximateNow(); if (fast_ack_after_quiescence_ && (approximate_now - time_of_previous_received_packet_) > @@ -1733,15 +1713,11 @@ QuicTime ack_time = clock_->ApproximateNow() + 0.125 * sent_packet_manager_.GetRttStats()->min_rtt(); - if (ShouldSetAckAlarm() || ack_alarm_->deadline() > ack_time) { + if (!ack_alarm_->IsSet() || ack_alarm_->deadline() > ack_time) { ack_alarm_->Update(ack_time, QuicTime::Delta::Zero()); } } else { - if (packet_generator_.deprecate_ack_bundling_mode()) { - MaybeSetAckAlarmTo(clock_->ApproximateNow()); - } else { - ack_queued_ = true; - } + MaybeSetAckAlarmTo(clock_->ApproximateNow()); } } @@ -1749,10 +1725,6 @@ time_of_previous_received_packet_ = time_of_last_received_packet_; } } - - if (ack_queued_) { - ack_alarm_->Cancel(); - } } void QuicConnection::ClearLastFrames() { @@ -1872,7 +1844,7 @@ return 0; } - ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); + ScopedPacketFlusher flusher(this); return packet_generator_.ConsumeCryptoData(level, write_length, offset); } @@ -1890,7 +1862,7 @@ // which decrypter will be used on an ack packet following a handshake // packet (a handshake packet from client to server could result in a REJ or a // SHLO from the server, leading to two different decrypters at the server.) - ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); + ScopedPacketFlusher flusher(this); return packet_generator_.ConsumeData(id, write_length, offset, state); } @@ -1901,7 +1873,7 @@ // Do not check congestion window for ping. return false; } - ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); + ScopedPacketFlusher flusher(this); const bool consumed = packet_generator_.ConsumeRetransmittableControlFrame(frame); if (packet_generator_.deprecate_queued_control_frames() && !consumed) { @@ -1930,7 +1902,7 @@ } // Flush stream frames of reset stream. if (packet_generator_.HasPendingStreamFramesOfStream(id)) { - ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); + ScopedPacketFlusher flusher(this); packet_generator_.FlushAllQueuedFrames(); } @@ -2039,7 +2011,7 @@ QUIC_DVLOG(1) << ENDPOINT << "time of last received packet: " << time_of_last_received_packet_.ToDebuggingValue(); - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); if (!framer_.ProcessPacket(packet)) { // If we are unable to decrypt this packet, it might be // because the CHLO or SHLO packet was lost. @@ -2100,7 +2072,7 @@ DCHECK(!writer_->IsWriteBlocked()); // Add a flusher to ensure the connection is marked app-limited. - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); WriteQueuedPackets(); if (received_packet_manager_.decide_when_to_send_acks()) { @@ -2123,7 +2095,6 @@ // Send an ACK now because either 1) we were write blocked when we last // tried to send an ACK, or 2) both ack alarm and send alarm were set to go // off together. - DCHECK(packet_generator_.deprecate_ack_bundling_mode()); SendAck(); } if (!session_decides_what_to_write()) { @@ -2143,7 +2114,7 @@ } { - ScopedPacketFlusher flusher(this, SEND_ACK_IF_QUEUED); + ScopedPacketFlusher flusher(this); visitor_->OnCanWrite(); } @@ -2166,7 +2137,7 @@ void QuicConnection::WriteAndBundleAcksIfNotBlocked() { if (!HandleWriteBlocked()) { - ScopedPacketFlusher flusher(this, SEND_ACK_IF_QUEUED); + ScopedPacketFlusher flusher(this); WriteIfNotBlocked(); } } @@ -2338,7 +2309,7 @@ // be moved outside of the loop. Also, CanWrite is not checked after the // generator is flushed. { - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); packet_generator_.FlushAllQueuedFrames(); } DCHECK(!packet_generator_.HasQueuedFrames()); @@ -2390,7 +2361,6 @@ } const QuicFrames QuicConnection::MaybeBundleAckOpportunistically() { - DCHECK(packet_generator_.deprecate_ack_bundling_mode()); QuicFrames frames; bool has_pending_ack = false; if (received_packet_manager_.decide_when_to_send_acks()) { @@ -2811,8 +2781,7 @@ } // The client should immediately ack the SHLO to confirm the handshake is // complete with the server. - if (perspective_ == Perspective::IS_CLIENT && !ack_queued_ && - ack_frame_updated()) { + if (perspective_ == Perspective::IS_CLIENT && ack_frame_updated()) { ack_alarm_->Update(clock_->ApproximateNow(), QuicTime::Delta::Zero()); } } @@ -2849,25 +2818,21 @@ ResetAckStates(); } - if (packet_generator_.deprecate_ack_bundling_mode()) { - QUIC_DVLOG(1) << ENDPOINT << "Sending an ACK proactively"; - QuicFrames frames; - frames.push_back(GetUpdatedAckFrame()); - if (!no_stop_waiting_frames_) { - QuicStopWaitingFrame stop_waiting; - PopulateStopWaitingFrame(&stop_waiting); - frames.push_back(QuicFrame(stop_waiting)); + QUIC_DVLOG(1) << ENDPOINT << "Sending an ACK proactively"; + QuicFrames frames; + frames.push_back(GetUpdatedAckFrame()); + if (!no_stop_waiting_frames_) { + QuicStopWaitingFrame stop_waiting; + PopulateStopWaitingFrame(&stop_waiting); + frames.push_back(QuicFrame(stop_waiting)); + } + if (received_packet_manager_.decide_when_to_send_acks()) { + if (!packet_generator_.FlushAckFrame(frames)) { + return; } - if (received_packet_manager_.decide_when_to_send_acks()) { - if (!packet_generator_.FlushAckFrame(frames)) { - return; - } - ResetAckStates(); - } else { - send_ack_when_on_can_write_ = !packet_generator_.FlushAckFrame(frames); - } + ResetAckStates(); } else { - packet_generator_.SetShouldSendAck(!no_stop_waiting_frames_); + send_ack_when_on_can_write_ = !packet_generator_.FlushAckFrame(frames); } if (consecutive_num_packets_with_no_retransmittable_frames_ < max_consecutive_num_packets_with_no_retransmittable_frames_) { @@ -2937,7 +2902,7 @@ void QuicConnection::SetDefaultEncryptionLevel(EncryptionLevel level) { if (level != encryption_level_ && packet_generator_.HasQueuedFrames()) { // Flush all queued frames when encryption level changes. - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); packet_generator_.FlushAllQueuedFrames(); } encryption_level_ = level; @@ -3104,12 +3069,11 @@ SetDefaultEncryptionLevel(GetConnectionCloseEncryptionLevel()); ClearQueuedPackets(); // If there was a packet write error, write the smallest close possible. - AckBundling ack_mode = (error == QUIC_PACKET_WRITE_ERROR) ? NO_ACK : SEND_ACK; - ScopedPacketFlusher flusher(this, ack_mode); + ScopedPacketFlusher flusher(this); // When multiple packet number spaces is supported, an ACK frame will be // bundled when connection is not write blocked. if (!SupportsMultiplePacketNumberSpaces() && - packet_generator_.deprecate_ack_bundling_mode() && ack_mode == SEND_ACK && + error != QUIC_PACKET_WRITE_ERROR && !GetUpdatedAckFrame().ack_frame->packets.Empty()) { SendAck(); } @@ -3344,15 +3308,13 @@ } void QuicConnection::MaybeSetAckAlarmTo(QuicTime time) { - DCHECK(packet_generator_.deprecate_ack_bundling_mode()); if (!ack_alarm_->IsSet() || ack_alarm_->deadline() > time) { ack_alarm_->Update(time, QuicTime::Delta::Zero()); } } QuicConnection::ScopedPacketFlusher::ScopedPacketFlusher( - QuicConnection* connection, - AckBundling ack_mode) + QuicConnection* connection) : connection_(connection), flush_and_set_pending_retransmission_alarm_on_delete_(false) { if (connection_ == nullptr) { @@ -3363,40 +3325,6 @@ flush_and_set_pending_retransmission_alarm_on_delete_ = true; connection->packet_generator_.AttachPacketFlusher(); } - if (connection_->packet_generator_.deprecate_ack_bundling_mode()) { - return; - } - - // If caller wants us to include an ack, check the delayed-ack timer to see if - // there's ack info to be sent. - if (ShouldSendAck(ack_mode)) { - if (!connection_->GetUpdatedAckFrame().ack_frame->packets.Empty()) { - QUIC_DVLOG(1) << "Bundling ack with outgoing packet."; - connection_->SendAck(); - } - } -} - -bool QuicConnection::ScopedPacketFlusher::ShouldSendAck( - AckBundling ack_mode) const { - DCHECK(!connection_->packet_generator_.deprecate_ack_bundling_mode()); - // If the ack alarm is set, make sure the ack has been updated. - DCHECK(!connection_->ack_alarm_->IsSet() || connection_->ack_frame_updated()) - << "ack_mode:" << ack_mode; - switch (ack_mode) { - case SEND_ACK: - return true; - case SEND_ACK_IF_QUEUED: - return connection_->ack_queued(); - case SEND_ACK_IF_PENDING: - return connection_->ack_alarm_->IsSet() || - connection_->stop_waiting_count_ > 1; - case NO_ACK: - return false; - default: - QUIC_BUG << "Unsupported ack_mode."; - return true; - } } QuicConnection::ScopedPacketFlusher::~ScopedPacketFlusher() { @@ -3410,44 +3338,41 @@ } if (flush_and_set_pending_retransmission_alarm_on_delete_) { - if (connection_->packet_generator_.deprecate_ack_bundling_mode()) { - if (connection_->received_packet_manager_.decide_when_to_send_acks()) { - const QuicTime ack_timeout = - connection_->use_uber_received_packet_manager_ - ? connection_->uber_received_packet_manager_ - .GetEarliestAckTimeout() - : connection_->received_packet_manager_.ack_timeout(); - if (ack_timeout.IsInitialized()) { - if (ack_timeout <= connection_->clock_->ApproximateNow() && - !connection_->CanWrite(NO_RETRANSMITTABLE_DATA)) { - // Cancel ACK alarm if connection is write blocked, and ACK will be - // sent when connection gets unblocked. - connection_->ack_alarm_->Cancel(); - } else { - connection_->MaybeSetAckAlarmTo(ack_timeout); - } + if (connection_->received_packet_manager_.decide_when_to_send_acks()) { + const QuicTime ack_timeout = + connection_->use_uber_received_packet_manager_ + ? connection_->uber_received_packet_manager_ + .GetEarliestAckTimeout() + : connection_->received_packet_manager_.ack_timeout(); + if (ack_timeout.IsInitialized()) { + if (ack_timeout <= connection_->clock_->ApproximateNow() && + !connection_->CanWrite(NO_RETRANSMITTABLE_DATA)) { + // Cancel ACK alarm if connection is write blocked, and ACK will be + // sent when connection gets unblocked. + connection_->ack_alarm_->Cancel(); + } else { + connection_->MaybeSetAckAlarmTo(ack_timeout); } } - if (connection_->ack_alarm_->IsSet() && - connection_->ack_alarm_->deadline() <= + } + if (connection_->ack_alarm_->IsSet() && + connection_->ack_alarm_->deadline() <= + connection_->clock_->ApproximateNow()) { + // An ACK needs to be sent right now. This ACK did not get bundled + // because either there was no data to write or packets were marked as + // received after frames were queued in the generator. + if (connection_->send_alarm_->IsSet() && + connection_->send_alarm_->deadline() <= connection_->clock_->ApproximateNow()) { - // An ACK needs to be sent right now. This ACK did not get bundled - // because either there was no data to write or packets were marked as - // received after frames were queued in the generator. - if (connection_->send_alarm_->IsSet() && - connection_->send_alarm_->deadline() <= - connection_->clock_->ApproximateNow()) { - // If send alarm will go off soon, let send alarm send the ACK. - connection_->ack_alarm_->Cancel(); - if (!connection_->received_packet_manager_ - .decide_when_to_send_acks()) { - connection_->send_ack_when_on_can_write_ = true; - } - } else if (connection_->SupportsMultiplePacketNumberSpaces()) { - connection_->SendAllPendingAcks(); - } else { - connection_->SendAck(); + // If send alarm will go off soon, let send alarm send the ACK. + connection_->ack_alarm_->Cancel(); + if (!connection_->received_packet_manager_.decide_when_to_send_acks()) { + connection_->send_ack_when_on_can_write_ = true; } + } else if (connection_->SupportsMultiplePacketNumberSpaces()) { + connection_->SendAllPendingAcks(); + } else { + connection_->SendAck(); } } connection_->packet_generator_.Flush(); @@ -3959,7 +3884,6 @@ void QuicConnection::ResetAckStates() { ack_alarm_->Cancel(); - ack_queued_ = false; stop_waiting_count_ = 0; num_retransmittable_packets_received_since_last_ack_sent_ = 0; num_packets_received_since_last_ack_sent_ = 0; @@ -3985,7 +3909,7 @@ if (!CanWrite(HAS_RETRANSMITTABLE_DATA)) { return MESSAGE_STATUS_BLOCKED; } - ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); + ScopedPacketFlusher flusher(this); return packet_generator_.AddMessageFrame(message_id, message); } @@ -4004,23 +3928,6 @@ return framer_.decrypter()->cipher_id(); } -bool QuicConnection::ShouldSetAckAlarm() const { - DCHECK(ack_frame_updated()); - if (ack_alarm_->IsSet()) { - // ACK alarm has been set. - return false; - } - if (GetQuicReloadableFlag(quic_fix_spurious_ack_alarm) && - packet_generator_.should_send_ack()) { - // If the generator is already configured to send an ACK, then there is no - // need to schedule the ACK alarm. The updated ACK information will be sent - // when the generator flushes. - QUIC_RELOADABLE_FLAG_COUNT(quic_fix_spurious_ack_alarm); - return false; - } - return true; -} - EncryptionLevel QuicConnection::GetConnectionCloseEncryptionLevel() const { if (perspective_ == Perspective::IS_CLIENT) { return encryption_level_;
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index dc97b07..186d50f 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -324,19 +324,6 @@ public QuicPacketGenerator::DelegateInterface, public QuicSentPacketManager::NetworkChangeVisitor { public: - // TODO(fayang): Remove this enum when deprecating - // quic_deprecate_ack_bundling_mode. - enum AckBundling { - // Send an ack if it's already queued in the connection. - SEND_ACK_IF_QUEUED, - // Always send an ack. - SEND_ACK, - // Bundle an ack with outgoing data. - SEND_ACK_IF_PENDING, - // Do not send ack. - NO_ACK, - }; - // Constructs a new QuicConnection for |connection_id| and // |initial_peer_address| using |writer| to write packets. |owns_writer| // specifies whether the connection takes ownership of |writer|. |helper| must @@ -526,10 +513,6 @@ bool ShouldGeneratePacket(HasRetransmittableData retransmittable, IsHandshake handshake) override; const QuicFrames MaybeBundleAckOpportunistically() override; - // Please note, this is not a const function. For logging purpose, please use - // ack_frame(). - const QuicFrame GetUpdatedAckFrame() override; - void PopulateStopWaitingFrame(QuicStopWaitingFrame* stop_waiting) override; // QuicPacketCreator::DelegateInterface char* GetPacketBuffer() override; @@ -541,6 +524,10 @@ void OnCongestionChange() override; void OnPathMtuIncreased(QuicPacketLength packet_size) override; + // Please note, this is not a const function. For logging purpose, please use + // ack_frame(). + const QuicFrame GetUpdatedAckFrame(); + // Called by the crypto stream when the handshake completes. In the server's // case this is when the SHLO has been ACKed. Clients call this on receipt of // the SHLO. @@ -707,16 +694,10 @@ // information to be sent. class QUIC_EXPORT_PRIVATE ScopedPacketFlusher { public: - // Setting |include_ack| to true ensures that an ACK frame is - // opportunistically bundled with the first outgoing packet. - // TODO(fayang): Remove |ack_mode| when deprecating - // quic_deprecate_ack_bundling_mode. - ScopedPacketFlusher(QuicConnection* connection, AckBundling ack_mode); + explicit ScopedPacketFlusher(QuicConnection* connection); ~ScopedPacketFlusher(); private: - bool ShouldSendAck(AckBundling ack_mode) const; - QuicConnection* connection_; // If true, when this flusher goes out of scope, flush connection and set // retransmission alarm if there is one pending. @@ -780,8 +761,6 @@ return termination_packets_.get(); } - bool ack_queued() const { return ack_queued_; } - bool ack_frame_updated() const; QuicConnectionHelperInterface* helper() { return helper_; } @@ -1116,14 +1095,12 @@ // num_retransmittable_packets_received_since_last_ack_sent_ etc. void ResetAckStates(); + void PopulateStopWaitingFrame(QuicStopWaitingFrame* stop_waiting); + // Enables multiple packet number spaces support based on handshake protocol // and flags. void MaybeEnableMultiplePacketNumberSpacesSupport(); - // Returns true if ack alarm is not set and there is no pending ack in the - // generator. - bool ShouldSetAckAlarm() const; - // Returns the encryption level the connection close packet should be sent at, // which is the highest encryption level that peer can guarantee to process. EncryptionLevel GetConnectionCloseEncryptionLevel() const; @@ -1276,10 +1253,6 @@ // Used when use_uber_received_packet_manager_ is true. UberReceivedPacketManager uber_received_packet_manager_; - // Indicates whether an ack should be sent the next time we try to write. - // TODO(fayang): Remove ack_queued_ when deprecating - // quic_deprecate_ack_bundling_mode. - bool ack_queued_; // How many retransmittable packets have arrived without sending an ack. // TODO(fayang): Remove // num_retransmittable_packets_received_since_last_ack_sent_ when deprecating @@ -1514,8 +1487,7 @@ // vector to improve performance since it is expected to be very small. std::vector<QuicConnectionId> incoming_connection_ids_; - // Indicates whether an ACK needs to be sent in OnCanWrite(). Only used when - // deprecate_ack_bundling_mode is true. + // Indicates whether an ACK needs to be sent in OnCanWrite(). // TODO(fayang): Remove this when ACK sending logic is moved to received // packet manager, and an ACK timeout would be used to record when an ACK // needs to be sent.
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 4cc121b..fcefc82 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -647,7 +647,7 @@ size_t total_length, QuicStreamOffset offset, StreamSendingState state) { - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); producer_.SaveStreamData(id, iov, iov_count, 0u, total_length); if (notifier_ != nullptr) { return notifier_->WriteOrBufferData(id, total_length, state); @@ -659,7 +659,7 @@ QuicStringPiece data, QuicStreamOffset offset, StreamSendingState state) { - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); if (!QuicUtils::IsCryptoStreamId(transport_version(), id) && this->encryption_level() == ENCRYPTION_INITIAL) { this->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); @@ -674,7 +674,7 @@ QuicStringPiece data, QuicStreamOffset offset, StreamSendingState state) { - ScopedPacketFlusher flusher(this, NO_ACK); + ScopedPacketFlusher flusher(this); DCHECK(encryption_level >= ENCRYPTION_ZERO_RTT); SetEncrypter(encryption_level, QuicMakeUnique<TaggingEncrypter>(0x01)); SetDefaultEncryptionLevel(encryption_level); @@ -1267,8 +1267,7 @@ void SendAckPacketToPeer() { EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); { - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::NO_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.SendAck(); } EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)) @@ -2930,8 +2929,7 @@ // Send two stream frames in 1 packet by queueing them. connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); { - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::SEND_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.SendStreamData3(); connection_.SendStreamData5(); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); @@ -2964,8 +2962,7 @@ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); { EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::SEND_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.SendStreamData3(); connection_.SendCryptoStreamData(); } @@ -2990,8 +2987,7 @@ { connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::SEND_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.SendCryptoStreamData(); connection_.SendStreamData3(); } @@ -3702,8 +3698,7 @@ EXPECT_CALL(visitor_, OnWriteBlocked()).Times(0); { - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::NO_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.CloseConnection(QUIC_PEER_GOING_AWAY, "no reason", ConnectionCloseBehavior::SILENT_CLOSE); @@ -3718,8 +3713,7 @@ EXPECT_CALL(visitor_, OnWriteBlocked()).Times(1); { - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::NO_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); // flusher's destructor will call connection_.FlushPackets, which should add // the connection to the write blocked list. } @@ -8015,8 +8009,7 @@ QuicStringPiece message_data(message); QuicMemSliceStorage storage(nullptr, 0, nullptr, 0); { - QuicConnection::ScopedPacketFlusher flusher(&connection_, - QuicConnection::SEND_ACK); + QuicConnection::ScopedPacketFlusher flusher(&connection_); connection_.SendStreamData3(); // Send a message which cannot fit into current open packet, and 2 packets // get sent, one contains stream frame, and the other only contains the
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc index 614182c..210e24d 100644 --- a/quic/core/quic_packet_generator.cc +++ b/quic/core/quic_packet_generator.cc
@@ -26,45 +26,21 @@ packet_creator_(server_connection_id, framer, random_generator, delegate), next_transmission_type_(NOT_RETRANSMISSION), flusher_attached_(false), - should_send_ack_(false), - should_send_stop_waiting_(false), random_generator_(random_generator), fully_pad_crypto_handshake_packets_(true), - deprecate_ack_bundling_mode_( - GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)), deprecate_queued_control_frames_( - deprecate_ack_bundling_mode_ && GetQuicReloadableFlag(quic_deprecate_queued_control_frames)) {} QuicPacketGenerator::~QuicPacketGenerator() { DeleteFrames(&queued_control_frames_); } -void QuicPacketGenerator::SetShouldSendAck(bool also_send_stop_waiting) { - DCHECK(!deprecate_ack_bundling_mode_); - if (packet_creator_.has_ack()) { - // Ack already queued, nothing to do. - return; - } - - if (also_send_stop_waiting && packet_creator_.has_stop_waiting()) { - QUIC_BUG << "Should only ever be one pending stop waiting frame."; - return; - } - - should_send_ack_ = true; - should_send_stop_waiting_ = also_send_stop_waiting; - SendQueuedFrames(/*flush=*/false); -} - bool QuicPacketGenerator::ConsumeRetransmittableControlFrame( const QuicFrame& frame) { QUIC_BUG_IF(IsControlFrame(frame.type) && !GetControlFrameId(frame)) << "Adding a control frame with no control frame id: " << frame; DCHECK(QuicUtils::IsRetransmittableFrame(frame.type)) << frame; - if (deprecate_ack_bundling_mode_) { - MaybeBundleAckOpportunistically(); - } + MaybeBundleAckOpportunistically(); if (deprecate_queued_control_frames_) { QUIC_RELOADABLE_FLAG_COUNT(quic_deprecate_queued_control_frames); if (packet_creator_.HasPendingFrames()) { @@ -95,9 +71,7 @@ QuicStreamOffset offset) { QUIC_BUG_IF(!flusher_attached_) << "Packet flusher is not attached when " "generator tries to write crypto data."; - if (deprecate_ack_bundling_mode_) { - MaybeBundleAckOpportunistically(); - } + MaybeBundleAckOpportunistically(); // To make reasoning about crypto frames easier, we don't combine them with // other retransmittable frames in a single packet. // TODO(nharper): Once we have separate packet number spaces, everything @@ -140,9 +114,7 @@ "generator tries to write stream data."; bool has_handshake = QuicUtils::IsCryptoStreamId(packet_creator_.transport_version(), id); - if (deprecate_ack_bundling_mode_) { - MaybeBundleAckOpportunistically(); - } + MaybeBundleAckOpportunistically(); bool fin = state != NO_FIN; QUIC_BUG_IF(has_handshake && fin) << "Handshake packets should never send a fin"; @@ -276,10 +248,8 @@ bool QuicPacketGenerator::CanSendWithNextPendingFrameAddition() const { DCHECK(HasPendingFrames() || packet_creator_.pending_padding_bytes() > 0); HasRetransmittableData retransmittable = - (should_send_ack_ || should_send_stop_waiting_ || - packet_creator_.pending_padding_bytes() > 0) - ? NO_RETRANSMITTABLE_DATA - : HAS_RETRANSMITTABLE_DATA; + packet_creator_.pending_padding_bytes() > 0 ? NO_RETRANSMITTABLE_DATA + : HAS_RETRANSMITTABLE_DATA; if (retransmittable == HAS_RETRANSMITTABLE_DATA) { DCHECK(!queued_control_frames_.empty()); // These are retransmittable. } @@ -294,8 +264,6 @@ if (!AddNextPendingFrame() && first_frame) { // A single frame cannot fit into the packet, tear down the connection. QUIC_BUG << "A single frame cannot fit into packet." - << " should_send_ack: " << should_send_ack_ - << " should_send_stop_waiting: " << should_send_stop_waiting_ << " number of queued_control_frames: " << queued_control_frames_.size(); if (!queued_control_frames_.empty()) { @@ -353,29 +321,12 @@ } bool QuicPacketGenerator::HasPendingFrames() const { - return should_send_ack_ || should_send_stop_waiting_ || - !queued_control_frames_.empty(); + return !queued_control_frames_.empty(); } bool QuicPacketGenerator::AddNextPendingFrame() { QUIC_BUG_IF(!flusher_attached_) << "Packet flusher is not attached when " "generator tries to write control frames."; - if (should_send_ack_) { - should_send_ack_ = !packet_creator_.AddSavedFrame( - delegate_->GetUpdatedAckFrame(), next_transmission_type_); - return !should_send_ack_; - } - - if (should_send_stop_waiting_) { - delegate_->PopulateStopWaitingFrame(&pending_stop_waiting_frame_); - // If we can't this add the frame now, then we still need to do so later. - should_send_stop_waiting_ = !packet_creator_.AddSavedFrame( - QuicFrame(pending_stop_waiting_frame_), next_transmission_type_); - // Return success if we have cleared out this flag (i.e., added the frame). - // If we still need to send, then the frame is full, and we have failed. - return !should_send_stop_waiting_; - } - QUIC_BUG_IF(queued_control_frames_.empty()) << "AddNextPendingFrame called with no queued control frames."; @@ -512,9 +463,7 @@ QuicMemSliceSpan message) { QUIC_BUG_IF(!flusher_attached_) << "Packet flusher is not attached when " "generator tries to add message frame."; - if (deprecate_ack_bundling_mode_) { - MaybeBundleAckOpportunistically(); - } + MaybeBundleAckOpportunistically(); const QuicByteCount message_length = message.total_length(); if (message_length > GetCurrentLargestMessagePayload()) { return MESSAGE_STATUS_TOO_LARGE; @@ -535,7 +484,6 @@ } void QuicPacketGenerator::MaybeBundleAckOpportunistically() { - DCHECK(deprecate_ack_bundling_mode_); if (packet_creator_.has_ack()) { // Ack already queued, nothing to do. return;
diff --git a/quic/core/quic_packet_generator.h b/quic/core/quic_packet_generator.h index 96d2cf2..2f82654 100644 --- a/quic/core/quic_packet_generator.h +++ b/quic/core/quic_packet_generator.h
@@ -69,11 +69,6 @@ // Called when there is data to be sent. Retrieves updated ACK frame from // the delegate. virtual const QuicFrames MaybeBundleAckOpportunistically() = 0; - // TODO(fayang): Remove these two interfaces when deprecating - // quic_deprecate_ack_bundling_mode. - virtual const QuicFrame GetUpdatedAckFrame() = 0; - virtual void PopulateStopWaitingFrame( - QuicStopWaitingFrame* stop_waiting) = 0; }; QuicPacketGenerator(QuicConnectionId server_connection_id, @@ -85,13 +80,6 @@ ~QuicPacketGenerator(); - // Indicates that an ACK frame should be sent. - // If |also_send_stop_waiting| is true, then it also indicates that a - // STOP_WAITING frame should be sent as well. - // The contents of the frame(s) will be generated via a call to the delegate - // CreateAckFrame() when the packet is serialized. - void SetShouldSendAck(bool also_send_stop_waiting); - // Consumes retransmittable control |frame|. Returns true if the frame is // successfully consumed. Returns false otherwise. bool ConsumeRetransmittableControlFrame(const QuicFrame& frame); @@ -245,8 +233,6 @@ packet_creator_.set_debug_delegate(debug_delegate); } - bool should_send_ack() const { return should_send_ack_; } - void set_fully_pad_crypto_hadshake_packets(bool new_value) { fully_pad_crypto_handshake_packets_ = new_value; } @@ -255,10 +241,6 @@ return fully_pad_crypto_handshake_packets_; } - bool deprecate_ack_bundling_mode() const { - return deprecate_ack_bundling_mode_; - } - bool deprecate_queued_control_frames() const { return deprecate_queued_control_frames_; } @@ -303,19 +285,6 @@ // True if packet flusher is currently attached. bool flusher_attached_; - // Flags to indicate the need for just-in-time construction of a frame. - // TODO(fayang): Remove these two booleans when deprecating - // quic_deprecate_ack_bundling_mode. - bool should_send_ack_; - bool should_send_stop_waiting_; - // If we put a non-retransmittable frame in this packet, then we have to hold - // a reference to it until we flush (and serialize it). Retransmittable frames - // are referenced elsewhere so that they can later be (optionally) - // retransmitted. - // TODO(fayang): Remove this when deprecating - // quic_deprecate_ack_bundling_mode. - QuicStopWaitingFrame pending_stop_waiting_frame_; - QuicRandom* random_generator_; // Whether crypto handshake packets should be fully padded. @@ -326,9 +295,6 @@ // flusher detaches. QuicPacketNumber write_start_packet_number_; - // Latched value of quic_deprecate_ack_bundling_mode. - const bool deprecate_ack_bundling_mode_; - // Latched value of quic_deprecate_queued_control_frames. const bool deprecate_queued_control_frames_; };
diff --git a/quic/core/quic_packet_generator_test.cc b/quic/core/quic_packet_generator_test.cc index de2c927..e996c4e 100644 --- a/quic/core/quic_packet_generator_test.cc +++ b/quic/core/quic_packet_generator_test.cc
@@ -47,8 +47,6 @@ bool(HasRetransmittableData retransmittable, IsHandshake handshake)); MOCK_METHOD0(MaybeBundleAckOpportunistically, const QuicFrames()); - MOCK_METHOD0(GetUpdatedAckFrame, const QuicFrame()); - MOCK_METHOD1(PopulateStopWaitingFrame, void(QuicStopWaitingFrame*)); MOCK_METHOD0(GetPacketBuffer, char*()); MOCK_METHOD1(OnSerializedPacket, void(SerializedPacket* packet)); MOCK_METHOD2(OnUnrecoverableError, void(QuicErrorCode, const std::string&)); @@ -119,8 +117,7 @@ bool ConsumeRetransmittableControlFrame(const QuicFrame& frame, bool bundle_ack) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && - !QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack()) { + if (!QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack()) { QuicFrames frames; if (bundle_ack) { frames.push_back(QuicFrame(&ack_frame_)); @@ -158,8 +155,7 @@ if (total_length > 0) { producer_->SaveStreamData(id, iov, iov_count, 0, total_length); } - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && - !QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && + if (!QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleAckOpportunistically()).Times(1); @@ -169,8 +165,7 @@ MessageStatus AddMessageFrame(QuicMessageId message_id, QuicMemSliceSpan message) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && - !QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && + if (!QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleAckOpportunistically()).Times(1); @@ -182,8 +177,7 @@ QuicStringPiece data, QuicStreamOffset offset) { producer_->SaveCryptoData(level, offset, data); - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && - !QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && + if (!QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack() && delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleAckOpportunistically()).Times(1); @@ -345,78 +339,6 @@ MOCK_METHOD1(OnFrameAddedToPacket, void(const QuicFrame&)); }; -TEST_F(QuicPacketGeneratorTest, ShouldSendAck_NotWritable) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - return; - } - delegate_.SetCanNotWrite(); - - generator_.SetShouldSendAck(false); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_FALSE(generator_.HasRetransmittableFrames()); -} - -TEST_F(QuicPacketGeneratorTest, ShouldSendAck_WritableAndShouldNotFlush) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - return; - } - StrictMock<MockDebugDelegate> debug_delegate; - - generator_.set_debug_delegate(&debug_delegate); - delegate_.SetCanWriteOnlyNonRetransmittable(); - - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - EXPECT_CALL(debug_delegate, OnFrameAddedToPacket(_)).Times(1); - - generator_.SetShouldSendAck(false); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_FALSE(generator_.HasRetransmittableFrames()); -} - -TEST_F(QuicPacketGeneratorTest, ShouldSendAck_WritableAndShouldFlush) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - return; - } - delegate_.SetCanWriteOnlyNonRetransmittable(); - - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - EXPECT_CALL(delegate_, OnSerializedPacket(_)) - .WillOnce(Invoke(this, &QuicPacketGeneratorTest::SavePacket)); - - generator_.SetShouldSendAck(false); - generator_.Flush(); - EXPECT_FALSE(generator_.HasQueuedFrames()); - EXPECT_FALSE(generator_.HasRetransmittableFrames()); - - PacketContents contents; - contents.num_ack_frames = 1; - CheckPacketContains(contents, 0); -} - -TEST_F(QuicPacketGeneratorTest, ShouldSendAck_MultipleCalls) { - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - return; - } - // Make sure that calling SetShouldSendAck multiple times does not result in a - // crash. Previously this would result in multiple QuicFrames queued in the - // packet generator, with all but the last with internal pointers to freed - // memory. - delegate_.SetCanWriteAnything(); - - // Only one AckFrame should be created. - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - EXPECT_CALL(delegate_, OnSerializedPacket(_)) - .Times(1) - .WillOnce(Invoke(this, &QuicPacketGeneratorTest::SavePacket)); - - generator_.SetShouldSendAck(false); - generator_.SetShouldSendAck(false); - generator_.Flush(); -} - TEST_F(QuicPacketGeneratorTest, AddControlFrame_NotWritable) { delegate_.SetCanNotWrite(); @@ -845,9 +767,6 @@ TEST_F(QuicPacketGeneratorTest, ConsumeDataLargeSendAckFalse) { delegate_.SetCanNotWrite(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - generator_.SetShouldSendAck(false); - } QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); const bool success = generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), @@ -863,10 +782,6 @@ delegate_.SetCanWriteAnything(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - } if (generator_.deprecate_queued_control_frames()) { generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), /*bundle_ack=*/false); @@ -905,22 +820,8 @@ return; } delegate_.SetCanNotWrite(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - generator_.SetShouldSendAck(true /* stop_waiting */); - } delegate_.SetCanWriteAnything(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // Set up frames to write into the creator when control frames are written. - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - EXPECT_CALL(delegate_, PopulateStopWaitingFrame(_)); - // Generator should have queued control frames, and creator should be empty. - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_FALSE(generator_.HasRetransmittableFrames()); - EXPECT_FALSE(creator_->HasPendingFrames()); - } - // Create a 10000 byte IOVector. CreateData(10000); EXPECT_CALL(delegate_, OnSerializedPacket(_)) @@ -947,9 +848,6 @@ TEST_F(QuicPacketGeneratorTest, NotWritableThenBatchOperations) { delegate_.SetCanNotWrite(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - generator_.SetShouldSendAck(false); - } QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); const bool consumed = generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), @@ -966,12 +864,6 @@ delegate_.SetCanWriteAnything(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // When the first write operation is invoked, the ack frame will be - // returned. - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - } if (generator_.deprecate_queued_control_frames()) { EXPECT_TRUE( generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), @@ -996,12 +888,8 @@ EXPECT_FALSE(generator_.HasPendingStreamFramesOfStream(3)); PacketContents contents; - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // ACK will be flushed by connection. - contents.num_ack_frames = 0; - } else { - contents.num_ack_frames = 1; - } + // ACK will be flushed by connection. + contents.num_ack_frames = 0; if (!VersionHasIetfQuicFrames(framer_.transport_version())) { contents.num_goaway_frames = 1; } else { @@ -1015,9 +903,6 @@ TEST_F(QuicPacketGeneratorTest, NotWritableThenBatchOperations2) { delegate_.SetCanNotWrite(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - generator_.SetShouldSendAck(false); - } QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); const bool success = generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), @@ -1033,13 +918,6 @@ delegate_.SetCanWriteAnything(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // When the first write operation is invoked, the ack frame will be - // returned. - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - } - { InSequence dummy; // All five frames will be flushed out in a single packet @@ -1072,12 +950,8 @@ // The first packet should have the queued data and part of the stream data. PacketContents contents; - if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // ACK will be sent by connection. - contents.num_ack_frames = 0; - } else { - contents.num_ack_frames = 1; - } + // ACK will be sent by connection. + contents.num_ack_frames = 0; contents.num_rst_stream_frames = 1; contents.num_stream_frames = 1; CheckPacketContains(contents, 0); @@ -1433,22 +1307,8 @@ QuicPacketCreatorPeer::SetPacketNumber(creator_, 1000); delegate_.SetCanNotWrite(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - generator_.SetShouldSendAck(true); - } delegate_.SetCanWriteAnything(); - if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { - // Set up frames to write into the creator when control frames are written. - EXPECT_CALL(delegate_, GetUpdatedAckFrame()) - .WillOnce(Return(QuicFrame(&ack_frame_))); - EXPECT_CALL(delegate_, PopulateStopWaitingFrame(_)); - // Generator should have queued control frames, and creator should be empty. - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_FALSE(generator_.HasRetransmittableFrames()); - EXPECT_FALSE(creator_->HasPendingFrames()); - } - // This will not serialize any packets, because of the invalid frame. EXPECT_CALL(delegate_, OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, _));
diff --git a/quic/core/quic_received_packet_manager.cc b/quic/core/quic_received_packet_manager.cc index 4512952..e324c38 100644 --- a/quic/core/quic_received_packet_manager.cc +++ b/quic/core/quic_received_packet_manager.cc
@@ -62,7 +62,6 @@ time_of_previous_received_packet_(QuicTime::Zero()), was_last_packet_missing_(false), decide_when_to_send_acks_( - GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && GetQuicReloadableFlag(quic_rpm_decides_when_to_send_acks)) {} QuicReceivedPacketManager::~QuicReceivedPacketManager() {}
diff --git a/quic/core/quic_received_packet_manager.h b/quic/core/quic_received_packet_manager.h index 8abe5f9..0a04f63 100644 --- a/quic/core/quic_received_packet_manager.h +++ b/quic/core/quic_received_packet_manager.h
@@ -186,8 +186,7 @@ // Last sent largest acked, which gets updated when ACK was successfully sent. QuicPacketNumber last_sent_largest_acked_; - // Latched value of quic_deprecate_ack_bundling_mode and - // quic_rpm_decides_when_to_send_acks. + // Latched value of quic_rpm_decides_when_to_send_acks. const bool decide_when_to_send_acks_; };
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index efa3144..754a512 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -611,8 +611,7 @@ return; } - QuicConnection::ScopedPacketFlusher flusher( - connection_, QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher flusher(connection_); if (control_frame_manager_.WillingToWrite()) { control_frame_manager_.OnCanWrite(); } @@ -754,8 +753,7 @@ // QUIC STOP_SENDING frame. Both sre sent to emulate // the two-way close that Google QUIC's RST_STREAM does. if (VersionHasIetfQuicFrames(connection_->transport_version())) { - QuicConnection::ScopedPacketFlusher flusher( - connection(), QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher flusher(connection()); control_frame_manager_.WriteOrBufferRstStream(id, error, bytes_written); control_frame_manager_.WriteOrBufferStopSending(error, id); } else { @@ -1700,8 +1698,7 @@ void QuicSession::RetransmitFrames(const QuicFrames& frames, TransmissionType type) { - QuicConnection::ScopedPacketFlusher retransmission_flusher( - connection_, QuicConnection::NO_ACK); + QuicConnection::ScopedPacketFlusher retransmission_flusher(connection_); SetTransmissionType(type); for (const QuicFrame& frame : frames) { if (frame.type == MESSAGE_FRAME) { @@ -1802,8 +1799,7 @@ } bool QuicSession::RetransmitLostData() { - QuicConnection::ScopedPacketFlusher retransmission_flusher( - connection_, QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher retransmission_flusher(connection_); // Retransmit crypto data first. bool uses_crypto_frames = QuicVersionUsesCryptoFrames(connection_->transport_version());
diff --git a/quic/core/quic_versions.cc b/quic/core/quic_versions.cc index 33faa3c..1989e98 100644 --- a/quic/core/quic_versions.cc +++ b/quic/core/quic_versions.cc
@@ -429,9 +429,7 @@ // Enable necessary flags. SetQuicFlag(FLAGS_quic_supports_tls_handshake, true); - // 60 is the highest multiple of 4 and one-byte variable length integer. SetQuicFlag(FLAGS_quic_headers_stream_id_in_v99, 60); - SetQuicReloadableFlag(quic_deprecate_ack_bundling_mode, true); SetQuicReloadableFlag(quic_rpm_decides_when_to_send_acks, true); SetQuicReloadableFlag(quic_use_uber_loss_algorithm, true); SetQuicReloadableFlag(quic_use_uber_received_packet_manager, true);
diff --git a/quic/core/uber_received_packet_manager_test.cc b/quic/core/uber_received_packet_manager_test.cc index d7d49ee..931db1f 100644 --- a/quic/core/uber_received_packet_manager_test.cc +++ b/quic/core/uber_received_packet_manager_test.cc
@@ -49,7 +49,6 @@ class UberReceivedPacketManagerTest : public QuicTest { protected: UberReceivedPacketManagerTest() { - SetQuicReloadableFlag(quic_deprecate_ack_bundling_mode, true); SetQuicReloadableFlag(quic_rpm_decides_when_to_send_acks, true); manager_ = QuicMakeUnique<UberReceivedPacketManager>(&stats_); clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1));
diff --git a/quic/quartc/quartc_session.cc b/quic/quartc/quartc_session.cc index 84396ad..ac908f9 100644 --- a/quic/quartc/quartc_session.cc +++ b/quic/quartc/quartc_session.cc
@@ -68,8 +68,7 @@ } void QuartcSession::ProcessSendMessageQueue() { - QuicConnection::ScopedPacketFlusher flusher( - connection(), QuicConnection::AckBundling::NO_ACK); + QuicConnection::ScopedPacketFlusher flusher(connection()); while (!send_message_queue_.empty()) { QueuedMessage& it = send_message_queue_.front(); const size_t message_size = it.message.length();
diff --git a/quic/test_tools/quic_test_client.cc b/quic/test_tools/quic_test_client.cc index 3e577a3..caeaa51 100644 --- a/quic/test_tools/quic_test_client.cc +++ b/quic/test_tools/quic_test_client.cc
@@ -352,8 +352,7 @@ } QuicSpdyClientSession* session = client()->client_session(); - QuicConnection::ScopedPacketFlusher flusher( - session->connection(), QuicConnection::SEND_ACK_IF_PENDING); + QuicConnection::ScopedPacketFlusher flusher(session->connection()); ssize_t ret = SendMessage(headers, "", /*fin=*/true, /*flush=*/false); QuicStreamId stream_id = GetNthClientInitiatedBidirectionalStreamId(
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc index 624e4f7..16c441c 100644 --- a/quic/test_tools/simple_session_notifier.cc +++ b/quic/test_tools/simple_session_notifier.cc
@@ -283,8 +283,7 @@ void SimpleSessionNotifier::RetransmitFrames(const QuicFrames& frames, TransmissionType type) { - QuicConnection::ScopedPacketFlusher retransmission_flusher( - connection_, QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher retransmission_flusher(connection_); connection_->SetTransmissionType(type); for (const QuicFrame& frame : frames) { if (frame.type == CRYPTO_FRAME) {
diff --git a/quic/test_tools/simulator/quic_endpoint.cc b/quic/test_tools/simulator/quic_endpoint.cc index 872a9b2..ed9d5c0 100644 --- a/quic/test_tools/simulator/quic_endpoint.cc +++ b/quic/test_tools/simulator/quic_endpoint.cc
@@ -386,8 +386,7 @@ void QuicEndpoint::WriteStreamData() { // Instantiate a flusher which would normally be here due to QuicSession. - QuicConnection::ScopedPacketFlusher flusher( - &connection_, QuicConnection::SEND_ACK_IF_QUEUED); + QuicConnection::ScopedPacketFlusher flusher(&connection_); while (bytes_to_transfer_ > 0) { // Transfer data in chunks of size at most |kWriteChunkSize|.