Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s2n-quic-transport): revert "improve amplification credits on client #1818" #1906

Merged
merged 1 commit into from
Aug 11, 2023
Merged

fix(s2n-quic-transport): revert "improve amplification credits on client #1818" #1906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions quic/s2n-quic-transport/src/connection/transmission.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,8 @@ impl<'a, 'sub, Config: endpoint::Config> tx::Message for ConnectionTransmission<
// end, so we check that next. Finally, if there is no ApplicationData or Handshake packet
// to transmit, the Initial packet itself will be padded.
let mut pn_space_to_pad = {
let needs_padding = if Config::ENDPOINT_TYPE.is_client() {
// if we're the client and haven't completed the handshake, we need to pad out packets
// in order to give the server as many amplification credits as possible.
!space_manager.is_handshake_confirmed()
} else {
// if we're the server, we only need to pad while we have the initial space
has_transmission(space_manager.initial(), transmission_constraint)
};
let needs_padding =
has_transmission(space_manager.initial(), transmission_constraint);

if !needs_padding {
// There is no Initial packet, so no padding is needed
Expand Down
19 changes: 1 addition & 18 deletions quic/s2n-quic-transport/src/transmission/early.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
ack::AckManager, contexts::WriteContext, endpoint, recovery, space::CryptoStream, transmission,
};
use core::ops::RangeInclusive;
use s2n_quic_core::{frame::Ping, packet::number::PacketNumberSpace};
use s2n_quic_core::packet::number::PacketNumberSpace;

pub struct Payload<'a, Config: endpoint::Config> {
pub ack_manager: &'a mut AckManager,
Expand All @@ -30,9 +30,6 @@ impl<'a, Config: endpoint::Config> super::Payload for Payload<'a, Config> {
"Early transmissions should not be used for MTU probing"
);

// record the starting capacity
let start_capacity = context.remaining_capacity();

let did_send_ack = self.ack_manager.on_transmit(context);

// Payloads can only transmit and retransmit
Expand All @@ -44,20 +41,6 @@ impl<'a, Config: endpoint::Config> super::Payload for Payload<'a, Config> {
// send PINGs last, since they might not actually be needed if there's an ack-eliciting
// frame already present in the payload
self.recovery_manager.on_transmit(context);

// In order to trigger the loss recovery mechanisms during the handshake make all packets
// ack-eliciting. This is especially true for the client in order to give the server more
// amplification credits.
//
// Only send a PING if:
// * We're not congestion limited
// * The packet isn't already ack-eliciting
// * Another frame was written to the context
if !context.ack_elicitation().is_ack_eliciting()
&& start_capacity != context.remaining_capacity()
{
let _ = context.write_frame(&Ping);
}
}

if did_send_ack {
Expand Down