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.

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

Remove direct Instant::now() calls #1828

Open
birneee opened this issue Jul 31, 2024 · 1 comment
Open

Remove direct Instant::now() calls #1828

birneee opened this issue Jul 31, 2024 · 1 comment

Comments

@birneee
Copy link

birneee commented Jul 31, 2024

Quiche currently uses Instant::now() directly in various places.
Moving time management from quiche to the applications would make the quiche state machine more deterministic, which would improve testability.
Also, the time granularity could be controlled by the application, potentially reducing the number of syscalls.

For example the signature of send(&mut self, out: &mut [u8]) could be changed to send(&mut self, out: &mut [u8], now: Instant).

Automated tests that require timed events would be simpler.
Example:

     #[test]
     /// Tests that old data is retransmitted on PTO.
     fn early_retransmit() {
         let mut buf = [0; 65535];
+        let mut now = EPOCH;
 
-        let mut pipe = testing::Pipe::new().unwrap();
+        let mut pipe = testing::Pipe::new(now).unwrap();
-        assert_eq!(pipe.handshake(), Ok(()));
+        now = pipe.handshake(now, Duration::from_millis(1)).unwrap();
 
         // Client sends stream data.
         assert_eq!(pipe.client.stream_send(0, b"a", false), Ok(1));
-        assert_eq!(pipe.advance(), Ok(()));
+        now = pipe.advance(now, Duration::from_millis(1)).unwrap();
 
         // Client sends more stream data, but packet is lost
         assert_eq!(pipe.client.stream_send(4, b"b", false), Ok(1));
-        assert!(pipe.client.send(&mut buf).is_ok());
+        assert!(pipe.client.send(&mut buf, now).is_ok());
 
         // Wait until PTO expires. Since the RTT is very low, wait a bit more.
-        let timer = pipe.client.timeout().unwrap();
+        let timer = pipe.client.timeout(now).unwrap();
-        std::thread::sleep(timer + time::Duration::from_millis(1));
+        now += timer + Duration::from_millis(1);

-        pipe.client.on_timeout();
+        pipe.client.on_timeout(now);
 
         let epoch = packet::Epoch::Application;
         assert_eq!(
             pipe.client
                 .paths
                 .get_active()
                 .expect("no active")
                 .recovery
                 .loss_probes(epoch),
             1,
         );
 
         // Client retransmits stream data in PTO probe.
-        let (len, _) = pipe.client.send(&mut buf).unwrap();
+        let (len, _) = pipe.client.send(&mut buf, now).unwrap();
         assert_eq!(
             pipe.client
                 .paths
                 .get_active()
                 .expect("no active")
                 .recovery
                 .loss_probes(epoch),
             0,
         );
 
         let frames =
             testing::decode_pkt(&mut pipe.server, &mut buf[..len]).unwrap();
 
         let mut iter = frames.iter();
 
         // Skip ACK frame.
         iter.next();
 
         assert_eq!(
             iter.next(),
             Some(&frame::Frame::Stream {
                 stream_id: 4,
                 data: stream::RangeBuf::from(b"b", 0, false),
             })
         );
         assert_eq!(pipe.client.stats().retrans, 1);
     }
@ruuda
Copy link

ruuda commented Nov 23, 2024

Passing in now externally would be nice for making the state machine deterministic to aid testing.

In addition, hard-coding the std::time::Instant type for deadlines is a bit unfortunate, because std::time::Instant does not expose its internal fields. This means that it is unsuitable for use with e.g. io_uring::Timeout with TimeoutFlags::ABS. It is possible to work around this by using Connection::timeout() instead of Connection::timeout_instant(), but that means there is an additional call to std::Instant::now() (inside Connection::timeout), which could be avoided with a time type that exposes the data that the kernel needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants