refactor: Some much needed renaming of variables mostly

And collecting some functions together. Split off from #338.

Author: flub

quinn-proto/src/connection/mod.rs

@@ -229,7 +229,7 @@ pub struct Connection {
     // Queued non-retransmittable 1-RTT data
     //
     /// If the CONNECTION_CLOSE frame needs to be sent
-    close: bool,
+    connection_close_pending: bool,
 
     //
     // ACK frequency
@@ -426,7 +426,7 @@ impl Connection {
             },
             timers: TimerTable::default(),
             authentication_failures: 0,
-            close: false,
+            connection_close_pending: false,
 
             ack_frequency: AckFrequencyState::new(get_max_ack_delay(
                 &TransportParameters::default(),
@@ -970,7 +970,7 @@ impl Connection {
             StateType::Draining | StateType::Closed => {
                 // self.close is only reset once the associated packet had been
                 // encoded successfully
-                if !self.close {
+                if !self.connection_close_pending {
                     self.app_limited = true;
                     return None;
                 }
@@ -979,7 +979,7 @@ impl Connection {
             _ => false,
         };
 
-        // Check whether we need to send an ACK_FREQUENCY frame
+        // Schedule an ACK_FREQUENCY frame if a new one needs to be sent.
         if let Some(config) = &self.config.ack_frequency_config {
             let rtt = self
                 .paths
@@ -1019,15 +1019,8 @@ impl Connection {
                 && self.remote_cids.contains_key(id)
         });
 
-        if let Some(challenge) = self.send_prev_path_challenge(now, buf, path_id) {
-            return Some(challenge);
-        }
-
-        // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that
-        // path validation can occur while the link is saturated.
-        // TODO(@divma): I have no idea what this comment is supposed to mean
-        if let Some(response) = self.send_off_path_path_response(now, buf, path_id) {
-            return Some(response);
+        if let Some(transmit) = self.poll_transmit_off_path(now, buf, path_id) {
+            return Some(transmit);
         }
 
         let mut space_id = match path_id {
@@ -1249,7 +1242,7 @@ impl Connection {
                 can_send.other,
                 self,
             )?;
-            last_packet_number = Some(builder.exact_number);
+            last_packet_number = Some(builder.packet_number);
             coalesce = coalesce && builder.can_coalesce;
 
             if space_id == SpaceId::Initial && (self.side.is_client() || can_send.other) {
@@ -1317,7 +1310,7 @@ impl Connection {
                 if space_id == self.highest_space {
                     // Don't send another close packet. Even with multipath we only send
                     // CONNECTION_CLOSE on a single path since we expect our paths to work.
-                    self.close = false;
+                    self.connection_close_pending = false;
                     // `CONNECTION_CLOSE` is the final packet
                     break;
                 } else {
@@ -1412,8 +1405,11 @@ impl Connection {
                 } else {
                     builder.finish_and_track(now, self, path_id, pad_datagram);
                 }
+
+                // If this is the first datagram we set the segment size to the size of the
+                // first datagram.
                 if transmit.num_datagrams() == 1 {
-                    transmit.clip_datagram_size();
+                    transmit.clip_segment_size();
                 }
             }
         }
@@ -1511,6 +1507,15 @@ impl Connection {
             return None;
         }
 
+        Some(self.build_transmit(path_id, transmit))
+    }
+
+    fn build_transmit(&mut self, path_id: PathId, transmit: TransmitBuf<'_>) -> Transmit {
+        debug_assert!(
+            !transmit.is_empty(),
+            "must not be called with an empty transmit buffer"
+        );
+
         let network_path = self.path_data(path_id).network_path;
         trace!(
             segment_size = transmit.segment_size(),
@@ -1532,7 +1537,7 @@ impl Connection {
             .udp_tx
             .on_sent(transmit.num_datagrams() as u64, transmit.len());
 
-        Some(Transmit {
+        Transmit {
             destination: network_path.remote,
             size: transmit.len(),
             ecn: if self.path_data(path_id).sending_ecn {
@@ -1545,7 +1550,23 @@ impl Connection {
                 _ => Some(transmit.segment_size()),
             },
             src_ip: network_path.local_ip,
-        })
+        }
+    }
+
+    /// poll_transmit logic for off-path data.
+    fn poll_transmit_off_path(
+        &mut self,
+        now: Instant,
+        buf: &mut Vec<u8>,
+        path_id: PathId,
+    ) -> Option<Transmit> {
+        if let Some(challenge) = self.send_prev_path_challenge(now, buf, path_id) {
+            return Some(challenge);
+        }
+        if let Some(response) = self.send_off_path_path_response(now, buf, path_id) {
+            return Some(response);
+        }
+        None
     }
 
     /// Returns the [`SpaceId`] of the next packet space which has data to send
@@ -1744,13 +1765,14 @@ impl Connection {
     /// Indicate what types of frames are ready to send for the given space
     ///
     /// *packet_size* is the number of bytes available to build the next packet.
-    /// *close* indicates whether a CONNECTION_CLOSE frame needs to be sent.
+    /// *connection_close_pending* indicates whether a CONNECTION_CLOSE frame needs to be
+    /// sent.
     fn space_can_send(
         &mut self,
         space_id: SpaceId,
         path_id: PathId,
         packet_size: usize,
-        close: bool,
+        connection_close_pending: bool,
     ) -> SendableFrames {
         let space = &mut self.spaces[space_id];
         let space_has_crypto = space.crypto.is_some();
@@ -1774,7 +1796,7 @@ impl Connection {
             can_send |= self.can_send_1rtt(path_id, frame_space_1rtt);
         }
 
-        can_send.close = close && space_has_crypto;
+        can_send.close = connection_close_pending && space_has_crypto;
 
         can_send
     }
@@ -2109,7 +2131,7 @@ impl Connection {
         if !was_closed {
             self.close_common();
             self.set_close_timer(now);
-            self.close = true;
+            self.connection_close_pending = true;
             self.state.move_to_closed_local(reason);
         }
     }
@@ -3170,7 +3192,7 @@ impl Connection {
                     ));
                 self.close_common();
                 self.set_close_timer(now);
-                self.close = true;
+                self.connection_close_pending = true;
             }
             return;
         }
@@ -3817,7 +3839,7 @@ impl Connection {
                 .get(&path_id)
                 .map(|p| p.data.network_path)
                 .unwrap_or(network_path);
-            self.close = network_path == path_remote;
+            self.connection_close_pending = network_path == path_remote;
         }
     }
 
@@ -4891,7 +4913,7 @@ impl Connection {
 
         if let Some(reason) = close {
             self.state.move_to_draining(Some(reason.into()));
-            self.close = true;
+            self.connection_close_pending = true;
         }
 
         if Some(number) == self.spaces[SpaceId::Data].for_path(path_id).rx_packet
@@ -5087,7 +5109,7 @@ impl Connection {
         path_exclusive_only: bool,
         builder: &mut PacketBuilder<'a, 'b>,
     ) {
-        let pn = builder.exact_number;
+        let pn = builder.packet_number;
         let is_multipath_negotiated = self.is_multipath_negotiated();
         let stats = &mut self.stats.frame_tx;
         let space = &mut self.spaces[space_id];

quinn-proto/src/connection/packet_builder.rs

@@ -25,7 +25,7 @@ pub(super) struct PacketBuilder<'a, 'b> {
     path: PathId,
     pub(super) partial_encode: PartialEncode,
     pub(super) ack_eliciting: bool,
-    pub(super) exact_number: u64,
+    pub(super) packet_number: u64,
     /// Is this packet allowed to be coalesced?
     pub(super) can_coalesce: bool,
     /// Smallest absolute position in the associated buffer that must be occupied by this packet's
@@ -93,11 +93,11 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
         }
 
         let space = &mut conn.spaces[space_id];
-        let exact_number = space.for_path(path_id).get_tx_number(&mut conn.rng);
-        let span = trace_span!("send", space = ?space_id, pn = exact_number, %path_id).entered();
+        let packet_number = space.for_path(path_id).get_tx_number(&mut conn.rng);
+        let span = trace_span!("send", space = ?space_id, pn = packet_number, %path_id).entered();
 
         let number = PacketNumber::new(
-            exact_number,
+            packet_number,
             space.for_path(path_id).largest_acked_packet.unwrap_or(0),
         );
         let header = match space_id {
@@ -171,7 +171,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
 
         qlog.header(
             &header,
-            Some(exact_number),
+            Some(packet_number),
             space_id,
             space_id == SpaceId::Data && conn.spaces[SpaceId::Data].crypto.is_none(),
             path_id,
@@ -182,7 +182,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
             space: space_id,
             path: path_id,
             partial_encode,
-            exact_number,
+            packet_number,
             can_coalesce: header.can_coalesce(),
             min_size,
             tag_len,
@@ -201,7 +201,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
             path: PathId::ZERO,
             partial_encode: PartialEncode::no_header(),
             ack_eliciting: true,
-            exact_number: 0,
+            packet_number: 0,
             can_coalesce: true,
             min_size: 0,
             tag_len: 0,
@@ -291,7 +291,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
             PadDatagram::ToMinMtu => self.pad_to(MIN_INITIAL_SIZE),
         }
         let ack_eliciting = self.ack_eliciting;
-        let exact_number = self.exact_number;
+        let packet_number = self.packet_number;
         let space_id = self.space;
         let (size, padded, sent) = self.finish(conn, now);
 
@@ -311,7 +311,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
         };
 
         conn.paths.get_mut(&path_id).unwrap().data.sent(
-            exact_number,
+            packet_number,
             packet,
             conn.spaces[space_id].for_path(path_id),
         );
@@ -372,7 +372,7 @@ impl<'a, 'b> PacketBuilder<'a, 'b> {
         self.partial_encode.finish(
             packet_buf,
             header_crypto,
-            Some((self.exact_number, self.path, packet_crypto)),
+            Some((self.packet_number, self.path, packet_crypto)),
         );
 
         let packet_len = self.buf.len() - encode_start;

quinn-proto/src/connection/transmit_buf.rs

@@ -132,15 +132,15 @@ impl<'a> TransmitBuf<'a> {
         self.num_datagrams += 1;
     }
 
-    /// Clips the datagram size to the current size
+    /// Clips the segment size to the current size
     ///
     /// Only valid for the first datagram, when the datagram might be smaller than the
     /// segment size. Needed before estimating the available space in the next datagram
     /// based on [`TransmitBuf::segment_size`].
     ///
     /// Use [`TransmitBuf::start_new_datagram_with_size`] if you need to reduce the size of
     /// the last datagram in a batch.
-    pub(super) fn clip_datagram_size(&mut self) {
+    pub(super) fn clip_segment_size(&mut self) {
         debug_assert_eq!(self.num_datagrams, 1);
         if self.buf.len() < self.segment_size {
             trace!(