Skip to content

Commit 75fd57c

Browse files
authored
fix(iroh-relay): Fix proptests, make Datagrams::segment_size be an Option<NonZeroU16> (#3404)
## Description The proptests were failing in some rare case where `Datagrams::segment_size` was generated as `Some(0)`. (For some reason we haven't hit that before?). I added the failing case hash to the proptest regressions file and fixed the problem. I also changed `Datagrams::segment_size` to be an `Option<NonZeroU16>` instead of `Option<u16>`, which checks this invariant on the type level now. ## Change checklist <!-- Remove any that are not relevant. --> - [x] Self-review.
1 parent db36c65 commit 75fd57c

File tree

3 files changed

+17
-12
lines changed

3 files changed

+17
-12
lines changed

iroh-relay/proptest-regressions/protos/relay.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@
66
# everyone who runs the test benefits from these saved cases.
77
cc 9295f5287162dfb180e5826e563c2cea08b477b803ef412ff8351eb5c3eb45ef # shrinks to frame = KeepAlive
88
cc 753aabcf8ae2b4e4a52f451d58339aab85a4b61108afdf4b9600f97b3a33bf42 # shrinks to frame = Health { problem: None }
9+
cc 2b45ae945ff922d4c3dfbad31a5e57c535c0ee2739906272f21ed77f8b862528 # shrinks to frame = Datagrams { remote_node_id: PublicKey(3b6a27bcceb6a42d62a3a8d02a6f0d73653215771de243a63ac048a18b59da29), datagrams: Datagrams { ecn: None, segment_size: Some(0), .. } }

iroh-relay/src/protos/relay.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
//! * server then sends [`FrameType::RelayToClientDatagram`] or [`FrameType::RelayToClientDatagramBatch`] to recipient
88
//! * server sends [`FrameType::NodeGone`] when the other client disconnects
99
10+
use std::num::NonZeroU16;
11+
1012
use bytes::{Buf, BufMut, Bytes, BytesMut};
1113
use iroh_base::{NodeId, SignatureError};
1214
use n0_future::time::Duration;
@@ -138,7 +140,7 @@ pub struct Datagrams {
138140
pub ecn: Option<quinn_proto::EcnCodepoint>,
139141
/// The segment size if this transmission contains multiple datagrams.
140142
/// This is `None` if the transmit only contains a single datagram
141-
pub segment_size: Option<u16>,
143+
pub segment_size: Option<NonZeroU16>,
142144
/// The contents of the datagram(s)
143145
#[debug(skip)]
144146
pub contents: Bytes,
@@ -159,7 +161,7 @@ impl Datagrams {
159161
let ecn = self.ecn.map_or(0, |ecn| ecn as u8);
160162
dst.put_u8(ecn);
161163
if let Some(segment_size) = self.segment_size {
162-
dst.put_u16(segment_size);
164+
dst.put_u16(segment_size.into());
163165
}
164166
dst.put(self.contents.as_ref());
165167
dst
@@ -185,11 +187,7 @@ impl Datagrams {
185187

186188
let segment_size = if is_batch {
187189
let segment_size = bytes.get_u16(); // length checked above
188-
if segment_size == 0 {
189-
None
190-
} else {
191-
Some(segment_size)
192-
}
190+
NonZeroU16::new(segment_size)
193191
} else {
194192
None
195193
};
@@ -519,7 +517,7 @@ mod tests {
519517
remote_node_id: client_key.public(),
520518
datagrams: Datagrams {
521519
ecn: Some(quinn::EcnCodepoint::Ce),
522-
segment_size: Some(6),
520+
segment_size: NonZeroU16::new(6),
523521
contents: "Hello World!".into(),
524522
},
525523
}
@@ -589,7 +587,7 @@ mod tests {
589587
dst_node_id: client_key.public(),
590588
datagrams: Datagrams {
591589
ecn: Some(quinn::EcnCodepoint::Ce),
592-
segment_size: Some(6),
590+
segment_size: NonZeroU16::new(6),
593591
contents: "Hello World!".into(),
594592
},
595593
}
@@ -668,7 +666,9 @@ mod proptests {
668666
)
669667
.prop_map(|(ecn, segment_size, data)| Datagrams {
670668
ecn,
671-
segment_size: segment_size.map(|ss| std::cmp::min(data.len(), ss) as u16),
669+
segment_size: segment_size
670+
.map(|ss| std::cmp::min(data.len(), ss) as u16)
671+
.and_then(NonZeroU16::new),
672672
contents: Bytes::from(data),
673673
})
674674
}

iroh/src/magicsock/transports/relay.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::{
22
io,
3+
num::NonZeroU16,
34
task::{Context, Poll},
45
};
56

@@ -104,7 +105,7 @@ impl RelayTransport {
104105
meta_out.stride = dm
105106
.datagrams
106107
.segment_size
107-
.map_or(dm.datagrams.contents.len(), |s| s as usize);
108+
.map_or(dm.datagrams.contents.len(), |s| u16::from(s) as usize);
108109
meta_out.ecn = None;
109110
meta_out.dst_ip = None; // TODO: insert the relay url for this relay
110111

@@ -314,7 +315,10 @@ fn datagrams_from_transmit(transmit: &Transmit<'_>) -> Datagrams {
314315
quinn_udp::EcnCodepoint::Ect1 => quinn_proto::EcnCodepoint::Ect1,
315316
quinn_udp::EcnCodepoint::Ce => quinn_proto::EcnCodepoint::Ce,
316317
}),
317-
segment_size: transmit.segment_size.map(|ss| ss as u16),
318+
segment_size: transmit
319+
.segment_size
320+
.map(|ss| ss as u16)
321+
.and_then(NonZeroU16::new),
318322
contents: Bytes::copy_from_slice(transmit.contents),
319323
}
320324
}

0 commit comments

Comments
 (0)