wasip3: Implement nonblocking UDP I/O #775
Conversation
|
I'll note that procedurally this is based on #774 for now. This requires a Wasmtime due to the reliance of cancellation with nonblocking I/O and cancellation does not work well on 41.0.0. Updating Wasmtime, however, requires updating the wasip3 snapshot, hence the dependency. @badeend you're likely pretty interested in this as well. Out of curiosity would you be up for reviewing this? I understand you haven't done a lot of stuff in wasi-libc so there's no need to boot up on all the context if you aren't willing/able to, but I figured I'd ask nonetheless! |
|
@alexcrichton Now that #774 is merged, would you mind rebasing this PR off |
| } | ||
| } | ||
|
|
||
| static int poll_impl(struct pollfd *fds, size_t nfds, int timeout) { |
There was a problem hiding this comment.
I'm not able to immediately point a finger to any particular place, but something tells me there may be trouble if fds contains multiple entries for the same fd. Especially surrounding the register/ready callbacks per descriptor.
There was a problem hiding this comment.
Agreed yeah, I suspect that'll have a lot of problems. I'll work on that.
There was a problem hiding this comment.
It is an edge case, so as long as libc doesn't silently corrupt itself it's fine by me.
This commit implements nonblocking I/O for UDP sockets, specifically with `send` and `recv`. This additionally fills out enough of `poll` to get various tests doing nonblocking I/O to work. The main intention of this commit is to do the first foray into implementing nonblocking I/O in wasi-libc for wasip3. Broadly-speaking nonblocking I/O is in two categories: one being UDP and the other being `stream<u8>`-based reads/writes. This implements only the UDP half, and defers `stream<u8>` (and thus TCP for example) to a future commit. The implementation is relatively gnarly given the wasip3 APIs we currently have, but this was expected. I've done my best to be as careful as I can in terms of managing state machines but I'm pretty likely to inevitably get something wrong, so this should be expected to be something that's iterated on.
alexcrichton
force-pushed
the
udp-nonblocking
branch
from
ebe2e29 to
e4382cf
Compare
badeend
left a comment
badeend
left a comment
There was a problem hiding this comment.
Quite some intricate logic, but I think it all checks out.
There was a problem hiding this comment.
Its been a while since the last time I worked on this file. So I want to leave behind a general note, not related to the task at hand:
I see the p3 implementation builds on top of the p2 "streams" infrastructure. Strictly speaking, this should be unnecessary. All this bookkeeping exists because in 0.2 implicit binds were forbidden as that would bypass the concept of network handles. 0.3 doesn't have network handles and fully relies on virtualization. Everything regarding udp_socket_state_t, UDP_SOCKET_STATE_**, udp_socket_streams_t, etc. can be considered p2-only. The p3 async bookkeeping could in theory also be direct members on udp_socket_t.
Just a piece of background info.
There was a problem hiding this comment.
That's a good point yeah, I'll file a follow-up issue for that. For now I think it's valuable to minimize p2/p3 divergence and there's no consequence AFAIK to manually following the same state machine. This should be pretty easy to adjust in the future as well.
| // Ensure there's a subtask started or a packet ready. If this is a nonblocking | ||
| // operation and a subtask was started then polling below isn't very useful, | ||
| // so go ahead and return `EWOULDBLOCK`. | ||
| bool started = wasip3_recv_start(socket, streams); |
There was a problem hiding this comment.
wasip3_recv_start also returns true when the subtask finished immediately without blocking. Ideally, this function shouldn't return EWOULDBLOCK for that case. To prevent a redundant roundtrip to poll.
There was a problem hiding this comment.
The intention was to avoid the redundant trip to poll yeah, and I think this was actually just a structural refactoring bug in wasip3_recv_start now fixed, basically it looked like it could return true without actually starting anything but I don't believe it could in practice.
There was a problem hiding this comment.
I see you updated wasip3_recv_start. But I think my original point still stands:
The sockets_method_udp_socket_receive call may finish immediately with WASIP3_SUBTASK_RETURNED. This call site here (in wasip3_recv_ready) will then return EWOULDBLOCK causing the caller to do a roundtrip to poll, even though data was immediately available.
I think the condition here should be:
- if (started && !should_block) {
+ if (!streams->recv_ready && !should_block) {WDYT?
There was a problem hiding this comment.
Ah I see, and this is a good point! I adjusted the condition a bit to still take started into account and to additionally expand the comment here. The purpose of this block was handle the opposite case you're thinking of, some work was started, it's not done, and we're nonblocking. In that case there's no point in polling again for something that just said it wasn't ready. You're right though that it incorrectly didn't handle the case you're thinking of, work was started and it immediately finished. Should be actually updated now!
| // Ensure there's a subtask started or a packet ready. If this is a nonblocking | ||
| // operation and a subtask was started then polling below isn't very useful, | ||
| // so go ahead and return `EWOULDBLOCK`. | ||
| bool started = wasip3_recv_start(socket, streams); |
There was a problem hiding this comment.
I see you updated wasip3_recv_start. But I think my original point still stands:
The sockets_method_udp_socket_receive call may finish immediately with WASIP3_SUBTASK_RETURNED. This call site here (in wasip3_recv_ready) will then return EWOULDBLOCK causing the caller to do a roundtrip to poll, even though data was immediately available.
I think the condition here should be:
- if (started && !should_block) {
+ if (!streams->recv_ready && !should_block) {WDYT?
badeend
left a comment
badeend
left a comment
There was a problem hiding this comment.
LGTM. My approval has no power here, so someone else will have to take a look also
| // its `ready` callback. The `ready` callback will dictate what to do | ||
| // with `event` and will call `__wasilibc_poll_ready` as appropriate. | ||
| // | ||
| // TODO: having to search through the list isn't great, but short of |
There was a problem hiding this comment.
I think we'll want to go straight to supporting e.g. epoll if/when performance is a priority.
3 participants
This commit implements nonblocking I/O for UDP sockets, specifically
with
sendandrecv. This additionally fills out enough ofpolltoget various tests doing nonblocking I/O to work. The main intention of
this commit is to do the first foray into implementing nonblocking I/O
in wasi-libc for wasip3. Broadly-speaking nonblocking I/O is in two
categories: one being UDP and the other being
stream<u8>-basedreads/writes. This implements only the UDP half, and defers
stream<u8>(and thus TCP for example) to a future commit.The implementation is relatively gnarly given the wasip3 APIs we
currently have, but this was expected. I've done my best to be as
careful as I can in terms of managing state machines but I'm pretty
likely to inevitably get something wrong, so this should be expected to
be something that's iterated on.