Traits for asynchronous UDP

[chrysn]

Closes: #71

---

This adds traits for UDP that were removed in #70, but built from scratch. These address, in one go:

• Their API is now socket oriented (no passing the stack to socket operations, which is essential for async as discussed in TcpClient trait for creating shared TCP/IP stack #69/Remove tcp stack #70).
• Type differentiation between connected, bound and unbound sockets (TCP: Typestate client and server sockets #63), following the pattern of TcpConnect established in Remove tcp stack #70. There are currently no plans to convert between those, as they might be completely different beasts on different platforms. (But maybe traits that allow an unbound or bound socket to accept() and spin off a bound one might be added later).
• Send and receive methods take local addresses precisely in the cases where that is sensible, addressing UDP: send with source address #40.
• Binding now takes any parts of the address, although not all OSes might support all patterns, addressing Bind to IP #41.

This is early WIP and only checked to the point where it compiles; no implementations or users have been written yet -- but I'd like to have it around early for discussion.

[chrysn]

Based on feedback from @Dirbaio, reduced to two trait sets now.

[chrysn]

Updated with experience from implementing it for std.

There is now a branch at https://gitlab.com/chrysn/std-embedded-nal/-/tree/embedded-nal-async-0.2-udp that implements this on std. (It's currently cheating on the local address part; that's being fixed).

[chrysn]

An aspect I expect to be confronted with during review is that it's hard to get local addresses on servers (basically the #40 cluster); std-embedded-nal cheating there is evidence of that hardship, and the currently proposed API has now way for implementations to easily chicken out. (They can always return an error on .bind_multiple() and make their Unbound AT effectively !, but that would make programs fail at run time rather than not build).

I claim that this is a feature: It contributes to the coherence of the ecosystem, as it won't fragment into different levels of UDP support. If we had a base level and then extension traits, we'd get a lot of incomplete UDP implementations, which do not suffice for "advanced" use cases like CoAP¹, QUIC², [edit: or anything with multicast], or subtly break in the presence of NAT or when first used on a sever with multiple addresses.

Note that in constrained stacks it'll be actually easier to implement this than on std, which is limited by how that design flaw has been baked into POSIX' sockets API, which is only worked around by RFC 3542. I plan to demonstrate this by implementing the traits also for RIOT's sockets (which in term abstract over several network stacks).

Footnotes

1. where this is a common issue ↩

2. not fully sure, but "If a client receives packets from an unknown server address, the client MUST discard these packets" sounds like it ↩

[chrysn]

One detail that came up during std-embedded-nal implementation is that the IETF/POSIX API doesn't report the port of the incoming package -- it only reports the IP address. The port can be queried extra (and is stored in this implementation). I still think that the API is easier if a SocketAddr is used consistently throughout, but it's a piece of possibly useless information that the compiler may not be able to remove.

[chrysn]

There is one change I'd be tempted to do, but may not fit with the general pattern of this library: At several places, rather than passing no_std_net::SocketAddr, we could allow libraries to use types that merely support conversion to and from there. Especially when implementing a UDP server, this should defer conversion complexity to when it is needed -- for example, a UDP server responding to incoming packets is likely not interested in the sender address as a full IP address, it will only need to roundtrip that datum (an opaque type will afford that) or maybe match by identity (we could ask PartialEq and Hash).

I mention it here and not in a separate issue because implementers of this API could benefit from it a lot. This is true both for the std-based implementation (which would pass around opaque PktInfo structs) and constrained ones (which might use a refcount into a neighbor table rather than the full address, and use that information again right away).

[chrysn]

Rebased, edited to account for #76 (which needed the trivial change from IP addresses' ::unspecified() to ::UNSPECIFIED), and squashed. rustfmt was applied to all commits, and trivial fixes that had been fixups were squashed into their commits, but other than that, history was kept. Reviewers should look at the complete changes unless they're interested in how this changed after the initial review. All changes in this branch so far should probably become a single commit before this is merged.

[lulf]

I think it looks pretty good! I left an inline comment as well.

I'm maybe a little worried that even though the UdpStack trait is supporting some advanced uses, it is hard to tell up front if implementations will end up with lots of todo!'s instead of supporting it all. I'm not sure what to do about that other than using multiple traits.

[lulf]

I don't understand how this differs from connect() in practice. If the local address is unspecified, isn't the only reasonable thing for the implementation to select one automatically like connect()? Unless it's a common thing to do, I would probably leave it out of the trait.

[chrysn]

As (for lack of async fn in traits) the connect() method is paired with the ConnectFuture type, we can't have provided methods. Were that not the case, this could be the default impl for connect(), but as things are, implementers who want to use the simple default can just set type ConnectFuture<'a> = ConnectFromFuture<'a> and fn connect(&self, remote) -> ... { self.connect_default(remote) } in lieu of using the provided method.

It is a distinct method with its distinct return type because calling .connect() will be a common occurrence, and implementers might want to optimize this over the provided default. (In a POSIX implementation, this might be saving a syscall).

[chrysn]

This is not documented here (because the item is hidden, because users should not rely on it), but in the documentation of the .connect() method.

[chrysn]

As for the concern about implementations with todo!()s -- it would be perfectly reasonable to split UdpStack into a UdpClientStack (that has .connect_from() and .connect()), a UdpClientStackSingleAddress and a UdpClientStackMultiAddress (with .bind_single() and .bind_multiple(), respectively). I have a preference for a single trait (or maybe two traits -- client and server) because it creates an incentive to be comprehensive -- sure, people can still document that it's incomplete and todo!() some methods, but that'd create a clearer sense of "this is something we should do but just didn't get around to yet" (but accept PRs) compared to "well we implement some, users better make do with what is here".

But in the end, that's an opinion coming from a place where having these traits is usually a necessity, and if the maintainers, reviewers and/or embedded-community prefer, I'll just split the trait. (I'd still keep the names distinct, though, to allow easy and turbofish-free operation on stacks that support all modes, and for users who just use embedded_nal_async::*;).

[chrysn]

Thinking on on the todo!() issue, I think it'll also help to point out that all our operations are fallible, so the unimplemented ones wouldn't panic, but "just" return -ENOSYS or whichever platform error there is for "we don't implement this".

That is unlike embedded-hal (where my impression is that we're pretty fine-grained in what the types advertise in terms of features). Given how network stacks are generally very runtime-ish compared to embedded hardware, I think that's OK. Even if we had a network stack were you'd take ownership of an address that's configured on the network interface, and then split that into 65k UDP ports to which access is handed out -- that IP address probably has a "lifetime" that is vastly distinct from our concept of lifetime. So to abstract from that: Network socket operations, even such as static as binding, are conceptually fallible; and to add to that, they may depend on runtime configuration (you think you could bind to ::ffff:192.168.0.1? echo 1 > /proc/sys/net/ipv6/bindv6only, now try again!). From these, I think that distinguishing between client, single-bind and multi-bind sockets is possible, but unreliable enough to not be worth the type proliferation.

(I'd be happy to be shown wrong here. Good thing is: Even if it turns out that far down the way we'd revisit this, API changes or splits would only concern the stack, whereas the connections would still be handled by the ConnectedUdp and UnconnectedUdp traits. Comparing once more to embedded-hal, by providing constructors for these from the stack we're already doing way more, as we could also just specify connected and unconnected sockets and leave it to the platform to provide setup. It's good we do it this way because it works. But maybe becoming stable on the construction side is a tad less urgent than becoming stable on the socket side.)

[lulf]

I don't have any further comments, so I think this looks good!

[eldruin]

Any opinions @MathiasKoch, @ryan-summers ?

[MathiasKoch]

Looks good to me. No objections 👍

[ryan-summers]

I have not had the opportunity to dive into async Rust yet, so I don't know if I can have much valuable input for async traits. I've been leaving the async stuff up to other async folk for now.

That being said, I'm on board for moving fast and breaking things at this point, since these are all new concepts. We can fix them as we try them out - there's not an abundance of users on the embedded-nal-async yet.

[eldruin]

Great, @chrysn could you add an entry to the changelog?

[Dirbaio]

We probably want to merge the AFIT ones #77 now that TCP uses AFIT.

[chrysn]

With #78 done, yes, that's the way to go. I'll merge 77 into this PR so it's a single consistent change, and update the change log.

[chrysn]

Updated to AFIT using the very content of #78 (and changelog added); probably warrants a final review. The tests using std-embedded-nal still work from its branch (after fixing the embedded-io version, which was bumped in 0.3).

[chrysn]

I think this is ready; anything else for me to do? (I can't push the merge button ;-) ).

[Dirbaio]

LGTM overall! Just 2 things.

1. The difference between bind_single and bind_multiple seems very subtle to me.
   The only difference is what they do when the addr is unspecified, right? One picks one
   address and listens on that, the other listens on all addresses?

It's confusing that you can call both with a non-unspecified addr, in which case
they behave the same. It's 2 ways of doing the same thing.

I wonder if it can be made clearer using an enum? Also, if
we add a version enum we can support the use case of "listen in both v4 and v6":

enum IPVersion {
	V4V6,
	V4,
	V6,
}
enum LocalAddress {
	/// The network stack picks one address out of all the local addresses,
	/// and the socket listens to just that one.
	PickOne(IPVersion),
	/// The socket listens on all local addresses of the specified version.
	All(IPVersion),
	/// The socket listens on the given address. Unspecified address is not allowed.
	Fixed(Address),
}
async fn bind(local_addr: LocalAddress, local_port: u16) -> ...

---

2. I think naming is a bit confusing too. I'd like to bikeshed a bit:

Receive methods always receive into a buffer, there's not much point in adding _into, I'd rename receive_into -> receive.

I think we should stay away from "connect/connected" terms for UDP, as they imply
some kind of handshake or acknowledgement from the remote side, which UDP doesn't
have.

For the socket kinds I'd use "bound", which is what docs are already using:

• trait ConnectedUdp -> BoundUdpSocket as in "bound to a single remote address"
• trait UnconnectedUdp -> UnboundUdpSocket as in "not bound to a remote address"

For the associated types, I think it'd be simpler to name them after the
traits. This loses the ability of bind_single and bind_multiple to return
different types, but I can't think of any case you'd want that. Network stacks
usually have a single type for all UDP sockets.

type BoundUdpSocket: BoundUdpSocket;
type UnboundUdpSocket: UnboundUdpSocket;

For the methods, perhaps do bound_socket() and unbound_socket() as well, if we do the enum thing from above? Or new_bound_socket(), new_unbound_socket()

[Dirbaio]

Also, I think the associated types should have <Error = Self::Error>. for consistency with the TCP trait.

[chrysn]

Ad 1, maybe an enum:

It's confusing that you can call both with a non-unspecified addr, in which case they behave the same. It's 2 ways of doing the same thing.

Going through an enum won't make that aspect easier; but I can try to emphasize that in the documentation (gist is "Use _single unless you really intend to bind to multiple addresses").

Ad 1, IPVersion

The current interface manages to delegate all handling of different IP versions to SocketAddr (as does the TCP version), I'd prefer to keep it that way.

Technically, you never "bind to IPv4 and IPv6", you only bind to IPv6, and allow the socket to also take requests from its legacy support. (It's just that some platforms implement that legacy support, and some don't).

If there's a use case for expressing an unspecified address that matches all addresses except V6MAPPED IPv4 addresses, I suggest that be added to SocketAddr. (Which we might have to fork from no_std_net (or std later) unless the maintainers agree that this has a place in SocketAddr).

Ad 1, "why distinct types for uniquely and multiply bound", that does have a good reason: An implementation can do different optimizations with addresses bound to one address (that is, eventually elide all comparisons on the local address), or even only implement one. That's also why there are two different associated types around (but requiring the same traits, so an implementation may chose to serve both with the same).

Network stacks usually have a single type for all UDP sockets.

Well because they're forced through oversimplified APIs that don't differentiate ;-) (on POSIX, even TCP and UDP are of the same type, and we still use distinct types here as well)

More seriously:

• There is a very concrete difference to be made on POSIX-ish systems. On a socket bound to multiple addresses, one needs to go through the recvmsg / PKTINFO mechanism in order to obtain, and through sendmsg / PKTINFO in order to set, the local address when receiving and sending, respectively. A socket bound to a single local address can forego these extra steps, and just recvfrom / sendto.
• Similarly, in RIOT-OS, when I know that there is just a single local address, I spare the stack the steps of looking up the local address and instead provide the one in the socket instance.
  Both in this and in the POSIX case I'd still be copying the local address from somewhere (because based on early reviews, the traits for the two versions for UnconnectedUdp were unified), but that copying the Rust compiler can see through, and remove when the application doesn't need it.
  (In the version before your original review on the matrix room, they even had different traits for their associated types; that'd have allowed removing that copy even without relying on LTO).
• There are UDP stacks (often those that are controlled through Hayes-style commands) that just can not obtain the information which address a packet was sent to. By having distinct types, they can communicate this by having failing bind_multiple implementation (maybe even backed by an uninhabitated ::MutliplyBound), and still have a clean version of UniquelyBound that behaves consistently with other stacks (because it can remember its one address in the socket type, and report it in the receive method).

---

ad 2 bikeshedding, I'm less obstinate there ;-) -- I'd just like to explain the rationale about the choices made so far (and if you insist, can change things):

• _into: I do have hopes to later extend this into zero-copy capable interfaces. If we now name the reception into a buffer, I don't know what I'd call the reception into an owned data structure (which on dropping frees it in the network stack).

• Connected is different from Bound as Bound is about the local address and Connected is about the remote address. I'm borrowing terminology from the POSIX API here (not that I'd recommend borrowing from there on a general base, but here I lack better ideas): "connected" (having a remote address set) is what a socket is after the connect() call has been issued, and "bound" (having a local address set) is what it is after bind() has been called.

[chrysn]

<Error = Self::Error>

Thanks, good catch, fixed.

[eldruin]

Thank you for your work and everybody else for the discussion.
Could you resolve the conflicts? Then I would merge it.