Add packet retransmission on timeout #47

PurpleMyst · 2020-10-05T23:51:01Z

Resolves #6

connorkuehl · 2020-10-07T22:49:43Z

Just wanted to let you know that this is on my radar. It's a bit of a larger PR so it might take me a little while longer to wrap my head around the changes and compile any feedback I have. Thanks! 🙂

PurpleMyst · 2020-10-07T22:53:14Z

Alright, I appreciate it. Take your time, I understand it's an hefty PR :)

connorkuehl

Really fantastic work, @PurpleMyst! 😀

I have comments inline, but a summary:

Move RetransmissionConfig into the connection module (the connection module will have to be made pub iirc)
Implement Default for RetransmissionConfig. This way we can get rid of all the Option stuff around the retransmission config as well as stop having to pass an Option RetransmissionConfig in type constructors.
1. Construct the Server with the default config, and add a method on Server to let callers replace the default config.
2. client::{Builder, Config} are a similar story, but update the Builder to have a default RetransmissionConfig and give it a method for callers to overwrite with their own config.
Reword the doc comment for RetransmissionConfig.

connorkuehl · 2020-10-09T22:38:33Z

src/lib.rs

+#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]
+pub struct RetransmissionConfig {
+    /// How long should we wait for a reply before retransmitting the last packet?
+    timeout: std::time::Duration,
+
+    /// How many times should we retransmit the last packet?
+    ///
+    /// Note that this is the number of *retransmissions*, not transmissions, so
+    /// this means that setting this to `Some(0)` means that the packet will still be
+    /// sent once.
+    ///
+    /// If this is set to `None`, the packet will be retransmitted indefinitely.
+    max_retransmissions: Option<usize>,
+}


Let's put this in in the connection module, which will need to be made pub iirc.

connorkuehl · 2020-10-09T22:44:27Z

src/client.rs

 }

 impl Builder<New> {
    /// Generates a Transfer ID (a bind address & port) and opens a `UdpSocket`
    /// for this connection.
-    pub fn new() -> Result<Self> {
+    pub fn new(retransmission_config: Option<RetransmissionConfig>) -> Result<Self> {


Instead of passing this as an argument to this constructor, let's add a method to impl Builder<ConnectTo> called with_retransmit(cfg: RetransmissionConfig) -> Result<Self> and just replace the default RetransmissionConfig.

connorkuehl · 2020-10-09T22:44:37Z

src/client.rs


 /// The initial state for building a `Client`.
 pub struct New {
    socket: UdpSocket,
+    retransmission_config: Option<RetransmissionConfig>,


Suggested change

retransmission_config: Option<RetransmissionConfig>,

One of my later comments suggests moving this only into ConnectTo and Client.

connorkuehl · 2020-10-09T22:45:12Z

src/client.rs

@@ -24,6 +26,7 @@ pub struct New {
 pub struct ConnectTo {
    server: Vec<SocketAddr>,
    socket: UdpSocket,
+    retransmission_config: Option<RetransmissionConfig>,


Suggested change

retransmission_config: Option<RetransmissionConfig>,

retransmission_config: RetransmissionConfig,

Let's construct this with a default retransmission config; one of my later comments suggests adding a method to Builder<ConnectTo> which will allow a caller to optionally replace the default retransmission config.

So, I think implementing Default for RetransmissionConfig is a good first step here.

connorkuehl · 2020-10-09T22:47:26Z

src/client.rs

@@ -35,18 +38,23 @@ pub struct Builder<T> {
 pub struct Client {
    server: Vec<SocketAddr>,
    socket: UdpSocket,
+    retransmission_config: Option<RetransmissionConfig>,


Suggested change

retransmission_config: Option<RetransmissionConfig>,

retransmission_config: RetransmissionConfig,

See other comments -- general idea is to use a default retransmission config and let caller decide if they want to replace the default by calling a method giving us their preferred config.

connorkuehl · 2020-10-09T23:09:34Z

src/connection.rs

+
+    #[test]
+    fn test_put_retransmits_data() {
+        const BOGUS_DATA: &[u8] = b"hey, look, listen";


connorkuehl · 2020-10-09T23:11:11Z

src/server.rs

+    pub fn new<A: ToSocketAddrs, P: AsRef<Path>>(
+        bind_to: A,
+        serve_from: P,
+        retransmission_config: Option<RetransmissionConfig>,
+    ) -> Result<Self> {


Suggested change

pub fn new<A: ToSocketAddrs, P: AsRef<Path>>(

bind_to: A,

serve_from: P,

retransmission_config: Option<RetransmissionConfig>,

) -> Result<Self> {

pub fn new<A: ToSocketAddrs, P: AsRef<Path>>(

bind_to: A,

serve_from: P,

) -> Result<Self> {

Similar feedback to Client. Once RetransmissionConfig implements Default, let's just have this constructor use its default value.

connorkuehl · 2020-10-09T23:11:27Z

src/server.rs

    pub fn random_port<A: AsRef<str>, P: AsRef<Path>>(
        ip_addr: A,
        serve_from: P,
+        retransmission_config: Option<RetransmissionConfig>,


Suggested change

pub fn random_port<A: AsRef<str>, P: AsRef<Path>>(

ip_addr: A,

serve_from: P,

retransmission_config: Option<RetransmissionConfig>,

pub fn random_port<A: AsRef<str>, P: AsRef<Path>>(

ip_addr: A,

serve_from: P,

Same feedback as above

connorkuehl · 2020-10-09T23:12:37Z

src/server.rs

    ) -> Result<(u16, Self)> {
        let mut rng = rand::thread_rng();
        let port: u16 = rng.gen_range(MIN_PORT_NUMBER, u16::MAX);
        let bind_to = format!("{}:{}", ip_addr.as_ref(), port);

-        Self::new(bind_to, serve_from).map(|server| (port, server))
+        Self::new(bind_to, serve_from, retransmission_config).map(|server| (port, server))


Let's add a new method on Server

fn with_transmission_config(&mut self, cfg: RetransmissionConfig)

and have it replace the default RetransmissionConfig it was constructed with

connorkuehl · 2020-10-09T23:18:38Z

src/lib.rs

+/// POD struct representing the configuration of the retransmission of packets
+// NB: this is a struct so that you can only specify max_retransmissions if you specify a time :>


Reword this to be less technical (I.e., move away from phrases like "POD struct"

Maybe something like "Describes the retransmission behavior for a TFTP connection"

PurpleMyst · 2020-10-10T12:04:21Z

What should the default RetransmissionConfig do? Currently passing None turns on Retransmission indefinitely.

connorkuehl · 2020-10-10T13:00:51Z

What should the default RetransmissionConfig do? Currently passing None turns on Retransmission indefinitely.

It can behave the same with an internal enum

enum Retransmit {
    Infinite,
    Finite(u64),
}

Or something else of your devising if you find something you like better!

PurpleMyst · 2020-10-11T11:46:46Z

Alright, I'll work on that. Are you sure that you want this in the connection module? We don't expose that currently, and exposing it just for RetransmissionConfig feels weird to me

connorkuehl

I'd like to investigate ways to move more of the retransmission logic into types. The check_retransmissions method has a number of side effects in addition to checking if we should retransmit. These side effects include updating the current_transmissions variable as well as possibly writing to the socket.

Suppose we had an enum like this:

pub enum Retransmit {
  /// Do not retransmit packets. Just error out.
    Nil,
    /// Retransmit packets a limited amount of times
    Finite {
        /// How many times should we retransmit?
        limit: usize,
    },
    Infinite,
}

It could impl Iterator and depending on the variant, it could help control the retransmissions. The implementation of the next() method for this trait means that:

If it's Nil, it can just return None.

If it's Finite it can decrement its limit and return Some(count) (the actual value is insignificant, so long as it is a Some and not a None otherwise iteration would halt)

And if it's Infinite then it will just always return Some.

Then, the retransmission loops can be written as:

for _ in self.retransmission_config.retransmit.iter_mut() {
// if successful, break out, blah blah
}

Or something similar depending on how one will access the contents of the RetransmissionConfig. I think this will help dispense with the check_retransmission method and allow us to manage retransmissions without being so aware of different bits of state.

Thoughts?

connorkuehl · 2020-10-11T23:31:09Z

src/client.rs

-pub struct New {
-    socket: UdpSocket,
-}
+pub struct New(());


Suggested change

pub struct New(());

pub struct New;

Why was the socket from this Builder step?

So that we could set the RetransmissionConfig at any time in the Builder.

Also, the reason there's one private field is to avoid people outside the crate constructing the New struct

src/connection.rs

connorkuehl · 2020-10-11T23:52:38Z

src/lib.rs

+#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]
+pub enum RetransmissionConfig {
+    /// Do not retransmit packets. Just error out.
+    NoRetransmission,
+
+    /// Retransmit packets indefinitely
+    ForeverAfter {
+        /// How long should we wait before retransmitting?
+        timeout: std::time::Duration,
+    },
+
+    /// Retransmit packets a limited amount of times
+    NTimesAfter {
+        /// How long should we wait before retransmitting?
+        timeout: std::time::Duration,
+
+        /// How many times should we retransmit?
+        limit: std::num::NonZeroUsize,
+    },
+}


Suggested change

#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]

pub enum RetransmissionConfig {

/// Do not retransmit packets. Just error out.

NoRetransmission,

/// Retransmit packets indefinitely

ForeverAfter {

/// How long should we wait before retransmitting?

timeout: std::time::Duration,

},

/// Retransmit packets a limited amount of times

NTimesAfter {

/// How long should we wait before retransmitting?

timeout: std::time::Duration,

/// How many times should we retransmit?

limit: std::num::NonZeroUsize,

},

}

pub enum Retransmit {

/// Do not retransmit packets. Just error out.

Nil,

/// Retransmit packets a limited amount of times

Finite {

/// How many times should we retransmit?

limit: std::num::NonZeroUsize,

},

Infinite,

}

#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)]

pub struct RetransmissionConfig {

timeout: std::time::Duration,

retransmit: Retransmit,

}

Move this into its own submodule under the connection submodule; it can be exported without making connection pub after all.

src/server.rs

PurpleMyst · 2020-10-12T11:57:26Z

First off, I'm not sure what the difference between my enum representation and yours is once you add back in the timeout Duration.

Secondly, I'm not sure I like the idea of using an Iterator simply for the fact that it makes the "send error packet on failure" case weird to handle. We'd need to break an enclosing labeled loop, put this into its own function that directly uses the next method, or use a loop with an if let and a break.

The check_retransmissions method is fine as is IMO, if you see it as less a pure check (one that would simply return a boolean) and more as an action and/or guard that is part of the retransmission process.

Co-authored-by: Connor Kuehl <[email protected]>

connorkuehl · 2020-10-12T12:55:01Z

First off, I'm not sure what the difference between my enum representation and yours is once you add back in the timeout Duration.

The suggested struct + enum representation doesn't duplicate the Duration across variant definitions.

Secondly, I'm not sure I like the idea of using an Iterator simply for the fact that it makes the "send error packet on failure" case weird to handle. We'd need to break an enclosing labeled loop, put this into its own function that directly uses the next method, or use a loop with an if let and a break.

I was just thinking out loud when I said Iterator. One of my goals for this project is to practice more of "thinking in types" in the hopes that it makes it easier to create these logical flows we have to deal with.

The check_retransmissions method is fine as is IMO, if you see it as less a pure check (one that would simply return a boolean) and more as an action and/or guard that is part of the retransmission process.

Have you experimented with an approach with fewer side effects?

PurpleMyst · 2020-10-12T16:55:34Z

The suggested struct + enum representation doesn't duplicate the Duration across variant definitions.

That's valid, but IMHO it doesn't matter that much because

It's laid out that way anyway by the rust compiler
It's more ergonomic for users this way

Have you experimented with an approach with fewer side effects?

I could move the incrementation of check_retransmisions out of the function itself, but we'd still need to increment it somewhere and remember to do so anytime we used the function. This way, it's baked in.

feat: add packet retransmission on timeout

6e61394

PurpleMyst marked this pull request as draft October 7, 2020 14:56

feat: add limit for number of retransmissions

f466561

PurpleMyst marked this pull request as ready for review October 7, 2020 15:45

refactor: extract retransmission check logic into function

e7796d8

connorkuehl requested changes Oct 9, 2020

View reviewed changes

PurpleMyst mentioned this pull request Oct 11, 2020

Handle duplicated requests #54

Open

refactor: change RetransmissionConfig into an enum

3675e8f

PurpleMyst requested a review from connorkuehl October 11, 2020 12:14

connorkuehl requested changes Oct 12, 2020

View reviewed changes

PurpleMyst and others added 2 commits October 12, 2020 14:14

docs: replace FIXME with link to github issue

11da7e7

Co-authored-by: Connor Kuehl <[email protected]>

style: remove blank line in Server struct fields

d104367

Co-authored-by: Connor Kuehl <[email protected]>

1c3t3a mentioned this pull request Oct 21, 2020

Add final packet dallying #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add packet retransmission on timeout #47

Add packet retransmission on timeout #47

PurpleMyst commented Oct 5, 2020

connorkuehl commented Oct 7, 2020

PurpleMyst commented Oct 7, 2020

connorkuehl left a comment

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

connorkuehl Oct 9, 2020

PurpleMyst commented Oct 10, 2020

connorkuehl commented Oct 10, 2020

PurpleMyst commented Oct 11, 2020

connorkuehl left a comment

connorkuehl Oct 11, 2020

PurpleMyst Oct 12, 2020

connorkuehl Oct 11, 2020

PurpleMyst commented Oct 12, 2020

connorkuehl commented Oct 12, 2020

PurpleMyst commented Oct 12, 2020

	retransmission_config: Option<RetransmissionConfig>,
	retransmission_config: RetransmissionConfig,

		/// POD struct representing the configuration of the retransmission of packets
		// NB: this is a struct so that you can only specify max_retransmissions if you specify a time :>

Add packet retransmission on timeout #47

Are you sure you want to change the base?

Add packet retransmission on timeout #47

Conversation

PurpleMyst commented Oct 5, 2020

connorkuehl commented Oct 7, 2020

PurpleMyst commented Oct 7, 2020

connorkuehl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurpleMyst commented Oct 10, 2020

connorkuehl commented Oct 10, 2020

PurpleMyst commented Oct 11, 2020

connorkuehl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurpleMyst commented Oct 12, 2020

connorkuehl commented Oct 12, 2020

PurpleMyst commented Oct 12, 2020