feat(embassy-net): Implement wait_recv_ready() + wait_send_ready() for UdpSocket and wait_read_ready() + wait_write_ready() for TcpSocket

[AnthonyGrondin]

This provides a readable() implementation for UdpSocket, as discussed on Matrix.
This API allows a library maintainer or the end user to wait / poll a socket to see if it is readable, without dequeuing any packet. This is useful for offloading packet processing in a different part of the application.

/cc @ivmarkov

[ivmarkov]

I think the implementation is correct.

My only regret is that this PR implements the absolute minimum necessary for edge-mdns to work on top of embassy-net, and we might need a similar, extra PR in future for Readable on top of a TCP socket.

How about extending the PR with a similar impl for the TCP sockets.
There - I believe - you would be able to just call the smoltcp peek(size: usize) method with size = 1

===========

Implementing a similar writable functionality for UDP/TCP sockets:

While we don't strictly need this, it would be nice to have I think if not for anything else, then for symmetry with readable.

Unfortunately, writable might require extending smoltcp itself.
What we are missing from it, is a method - can_enqueue on smoltcp's PacketBuffer and then also an extra method - is_writable on smotcp's udp::Socket and tcp::Socket, which just calls PacketBuffer::can_enqueue.

[AnthonyGrondin]

I'd be more than happy to implement the TCP part and take a look at implementing a writable() API. I think we could also add similar APIs to raw sockets.

The issue is that I don't have any code to test the TCP socket, for readable and writable (same for raw).

In edge-net for example, the Readable trait is only ever used for edge-mdns with UDP sockets.

[AnthonyGrondin]

Actually, looking at it, I think TcpSocket already has an API to check if we can read from the socket:
It uses can_recv(&self) -> bool from smoltcp, which internally check if there is data in the rx buffer.

embassy/embassy-net/src/tcp.rs

Lines 392 to 396 in 0ede847

	/// Get whether the socket is ready to receive data, i.e. whether there is some pending data in the receive buffer.
	pub fn can_recv(&self) -> bool {
	self.io.with(|s, _| s.can_recv())
	}
	}

I first believed may_send would gives us the functionality we need for writable:

embassy/embassy-net/src/tcp.rs

Lines 379 to 382 in 0ede847

	/// Get whether the socket is ready to send data, i.e. whether there is space in the send buffer.
	pub fn may_send(&self) -> bool {
	self.io.with(|s, _| s.may_send())
	}

BUT

The documentation is misleading, as smoltcp::socket::tcp::Socket::may_send() states the following:

/// However, it does not make any guarantees about the state
/// of the transmit buffer, and even if it returns true, [send](#method.send) may
/// not be able to enqueue any octets

[AnthonyGrondin]

So I'm not familiar with poll_fn, but smoltcp states the following about registering wakers:

Only one waker can be registered at a time. If another waker was previously registered, it is overwritten and will no longer be woken.

Would calling readable() / writable() multiple times simultaneously cause the previous call to never resolve?

[ivmarkov]

So I'm not familiar with poll_fn, but smoltcp states the following about registering wakers:

Only one waker can be registered at a time. If another waker was previously registered, it is overwritten and will no longer be woken.

Would calling readable() / writable() multiple times simultaneously cause the previous call to never resolve?

TLDR: No, but the two tasks, where each is e.g. calling readable().await will start to fight each other by each one re-registering its waker, which will result in high CPU usage.

Elaboration:

Formally speaking, that's not a problem and the code would still work correctly. This does not mean you should do it (because of the high CPU usage).

(UPDATE: Removed a paragraph which is about read NOT readable)

However - yes - at least my primary use case of readable is actually that it is used by more than one future!
As long as all the futures are in a single task, that would be no problem, as the waker is a single one.

If the futures are each spawned in a separate task - then - yes - there will be high-usage CPU dragons. :-)

[ivmarkov]

Argh, I need to take some rest. :-)

No. My case is NOT about two futures calling readable concurrently. It is about two futures (or tasks) sharing a single data buffer - yet - operating on two separate sockets. :-)

So readable is just like read after all - I don't anticipate there would be use cases where two futures (or tasks) will be interested whether the same socket is readable. But then again - that would work fine without high CPU if the two futures are in the same task (i.e. select).

[Dirbaio]

For TCP we should implement the official embedded-io ReadReady, WriteReady.

If we want to extend that with "async wait for", i'd name them poll_* and wait_*. It's more clear than async fn readable(), the name suggests it'd return a bool, as in "is this readable?". So:

// inherent methods
fn read_ready(&self) -> bool;
fn poll_read_ready(&self, cx: &mut Context) -> Poll<()>;
async fn wait_read_ready(&self);
fn write_ready(&self) -> bool;
fn poll_write_ready(&self, cx: &mut Context) -> Poll<()>;
async fn wait_write_ready(&self);

// ReadReady trait impl
fn read_ready(&self) -> bool;

// WriteReady trait impl
fn write_ready(&self) -> bool;

The API for UDP should be the same for consistency, except:

• it should NOT implement the traits, for the same reason we don't implement the main Read/Write traits: udp Is NOT a byte stream
• It uses "send" and "recv", not "read" and "write", so the equivalent "_ready" methods should be consistent with that.

so i'd suggest:

fn recv_ready(&self) -> bool;
fn poll_recv_ready(&self, cx: &mut Context) -> Poll<()>;
async fn wait_recv_ready(&self);
fn send_ready(&self) -> bool;
fn poll_send_ready(&self, cx: &mut Context) -> Poll<()>;
async fn wait_send_ready(&self);

[ivmarkov]

Not that I've been asked, but I'm fine with any names. The readable naming came from here.

[AnthonyGrondin]

I've renamed the functions to use the them poll_* and wait_* naming conventions.

It's only missing:

fn poll_send_ready(&self, cx: &mut Context) -> Poll<()>;
async fn wait_send_ready(&self);

for UDP to have the full readable / writable async wait on both TCP and UDP sockets.

I'll have to check what's the best way to check if a UDP connection is ready to send

[Dirbaio]

it's .can_send() on smoltcp UdpSocket.

[AnthonyGrondin]

it's .can_send() on smoltcp UdpSocket.

I can use udp::Socket::can_send() provided by smoltcp, but it only checks whether the tx buffer is full or not. It doesn't guarantee that calling send() would resolve successfully, as it will return Err(Error::Unadressable) if the local or remote port, or remote address are unspecified.

[ivmarkov]

@Dirbaio @AnthonyGrondin Is there anything stopping this from merging?

I have this one comment which might be a concern. Hopefully, I just misunderstand the smoltcp API.

[Dirbaio]

I have #3368 (comment) which might be a concern. Hopefully, I just misunderstand the smoltcp API.

okay i've checked smoltcp's source and this seems to be a bug indeed. Filed smoltcp-rs/smoltcp#1004, will fix later.