SocketSet is "too opinionated"

[Dirbaio]

When doing the async/await wrappers I'm running into some design issues whose root cause I think is SocketSet.

Overview of what I'm trying to do:

• It has to work without alloc.
• There's one async task running the network stack (polling the iface)
• There can be N tasks using the network stack.

I want each task to be able to create its own Sockets and register them into the network stack. This pretty much requires the stack, including the SocketSet, to be 'static. This requires socket buffers to be 'static, which rules out tasks allocating their own socket buffers. All I can think of is tasks storing their buffers in static muts with unsafe which is not fun at all..

One way around this is to have tasks own the buffers AND the sockets. The network stack only has pointers to the currently-alive sockets (doesn't own them), and they get registered on creation and unregistered on drop (this can be made safe with Pin, don't worry!)

This is not possible with the current SocketSet though. Hence the "too opinionated".

Looking at the iface code, the only thing it needs from SocketSet is iterating through the sockets. Do you think it's a good idea to change Interface to take a impl IntoIter<Item=&mut Socket> instead of a SocketSet? This way users can choose how they store their sockets.

[whitequark]

The reason SocketSet came into existence is to make it possible to check socket readiness in a way that is more efficient than just iterating through every socket (which is what smoltcp currently does). But this never came into existence.

The network stack only has pointers to the currently-alive sockets (doesn't own them), and they get registered on creation and unregistered on drop

This seems reasonable. Can you sketch out the interface you're suggesting in rough terms? Assuming you are suggesting here to change the ownership semantics of SocketSet.

this can be made safe with Pin, don't worry!

smoltcp predates Pin, I believe.

Do you think it's a good idea to change Interface to take a impl IntoIter<Item=&mut Socket> instead of a SocketSet?

That would lock us into the poll function iterating through every socket to check readiness forever (or until we break the interface), right? I don't think that's desirable.

[Dirbaio]

Can you sketch out the interface you're suggesting in rough terms? Assuming you are suggesting here to change the ownership semantics of SocketSet.

Well, what I was proposing is leaving SocketSet unchanged, but changing Interface so it can take "any iterable thing", which could be the current SocketSet or a custom thing made by the user, but...

That would lock us into the poll function iterating through every socket to check readiness forever (or until we break the interface), right? I don't think that's desirable.

Yep, you're totally right, I didn't think about this.

Maybe changing SocketSet to not own the sockets is the answer?

What I had in mind is something like this:

pub struct Set<'a, 'b: 'a, 'c: 'a + 'b> {
    sockets: ManagedSlice<'a, Option<*mut Socket<'b, 'c>>>
}
impl<'a, 'b: 'a, 'c: 'a + 'b> Set<'a, 'b, 'c> {
    pub fn new(..) {..}
    pub fn add(&mut self, socket: &'a mut Socket<'b, 'c>) -> Handle {..}
    pub fn remove(&mut self, handle: Handle) {..}

    /// Safety:
    /// - The socket must stay alive at the same memory address for lifetime 'a, or until it's removed.
    /// - you may not hold a &mut or a & to it while calling iface.poll() with this socketset
    pub unsafe fn add_unchecked(&mut self, *mut Socket<'b, 'c>) -> Handle {..}

   // etc
}

Regular non-async code would work nearly the same as now, except it passes a borrowed socket instead of an owned socket. The socket stays in a local var that stays borrowed until the SocketSet is destroyed. No need for unsafe.

For async code, the socket would be created in the async task's stack, registering with add_unchecked and then removed on drop. This would be done by the "async wrapper" so the end user doesn't have to use unsafe. The wrapper would require the user to pin it before registering, so it's safe (Pin guarantees that the socket is not moved, and that Drop is called).

[whitequark]

If you can actually implement it (I assume you can but these things can be really tricky) I'm all for this solution.

[Dirbaio]

I found a workaround... It's not pretty, but the unowned SocketSet wasn't going to be pretty either, so I'm not sure what's best.

Task owns the buffers, netstack owns the sockets. The trick is to transmute the buffers to 'static, and remove the socket from the netstack on drop, so the buffers aren't actually used beyond their real lifetimes.

https://gist.github.com/Dirbaio/199053cf1c61e94059f77fd1f769b28f

Requires no changes to smoltcp, You get to use it like this which is pretty clean and ergonomic imo :D

    let socket = TcpSocket::<U1024, U1024>::new();
    pin_mut!(socket);

    socket.as_mut().set_timeout(Some(SmolDuration::from_secs(1)));

    info!("connecting...");
    let remote_endpoint = (Ipv4Address::new(10, 42, 0, 1), 8000);
    socket.as_mut().connect(remote_endpoint, 49500).await.unwrap();

    socket.write_all(b"Hello!\n").await.unwrap();

    // do more stuff

    // socket is dropped here

[whitequark]

The trick is to transmute the buffers to 'static, and remove the socket from the netstack on drop, so the buffers aren't actually used beyond their real lifetimes.

Well, my policy is to not have unsafe in core smoltcp code, so I think that's just not going to work.

[Dirbaio]

Unsafe is only needed in these async wrappers, not in core smoltcp. It's also possible to write async wrappers without unsafe but they'd be less ergonomic.

[whitequark]

Ah, gotcha. By "unowned SocketSet wasn't going to be pretty either" do you mean the earlier suggestion you had that would take impl IntoIter<Item=&mut Socket>?

[Dirbaio]

no, I meant the "changing SocketSet to not own Sockets" from this comment. I don't think it's possible without unsafe, so if we don't want any unsafe in smoltcp at all then the way to go is the one from this comment...

[whitequark]

Hm. What about this compromise: having Interface take either SocketSet or impl IntoIter<Item=&mut Socket>? Or even turning SocketSet into a trait which we would be free to extend later with methods to query readiness.

[lattice0]

Hi @Dirbaio, if your goals are:

• It has to work without alloc.
• There's one async task running the network stack (polling the iface)
• There can be N tasks using the network stack

then wouldn't a simple function

iface.poll(socket: &mut Socket)

suffice? Then are free to poll in any way you want.

Another option would be to make SocketSet hold references to the sockets, so I can pass only the sockets I want at the time, effectively polling only the ones needed, leaving others free.

My concern is kinda like yours: I want the sockets to be in their own threads, and just poll the ones that are free.

[Dirbaio]

The problem with iface.poll(socket: &mut Socket) is when processing ingress. poll calls device.receive(), which returns a packet. That packet can be for any active socket. smoltcp needs to loop over all sockets to find the matching one.

Either way, our issues are different. In my case I want to use sockets from different async tasks, but all running from an executor that runs all tasks in the same thread. That's much easier than what you're doing (using sockets from different threads).

In the single-threaded case you don't even need mutexes (I'm currently experimenting with just one RefCell for a struct containing the Interface and the SocketSet, it's turning out fine so far).

[lattice0]

@Dirbaio got it. But wouldn't passing just the desired socket make iface.poll iterate just over the needed socket?

for mut tcp_socket in sockets.iter_mut().filter_map(TcpSocket::downcast) {

instead of iterating over sockets it'd simply make tcp_socket = socket.

Anyways, I could also create lots of SocketSet's each containing its own socket, and then poll each SocketSet separately. What you think of that? Won't it work in your case too?

[Dirbaio]

It won't work. It will drop the packet if it's for a socket other than the current one. The packet is then lost forever, it won't be picked up when you later poll the other socket.

[Dirbaio]

Hello! As part of issue cleanup I'm closing inactive or already-answered "question" issues, since they're not actionable.

Note this doesn't mean questions aren't welcome. They still are! It's just that they shouldn't stay open forever.