badeend opened PR #7895 from badeend:tcp-tokio
to bytecodealliance:main
:
Full discussion: https://bytecodealliance.zulipchat.com/#narrow/stream/217126-wasmtime/topic/tokio.20always.20reports.20readiness
TLDR: Our implementation was flawed. It called
tokio::net::TcpStream::try_from(...)
directly after creating the socket. That triggers Tokio to callepoll_ctl
to add it to the poll list. Linux sees that the socket is not connected (yet) and emits EPOLLHUP. Tokio interprets that to be a "final" state and never recovers from it. All future attempts to await I/O readiness immediately succeed.This PR moves away from rustix and uses Tokio's own methods for constructing, binding, connecting, listening & accepting sockets. It still uses rustix for socket options
badeend requested wasmtime-core-reviewers for a review on PR #7895.
badeend requested alexcrichton for a review on PR #7895.
alexcrichton submitted PR review:
Nice! One very small comment but otherwise everything looks as I'd expect here :+1:
alexcrichton submitted PR review:
Nice! One very small comment but otherwise everything looks as I'd expect here :+1:
alexcrichton created PR review comment:
How come this does a
spawn
?
badeend submitted PR review.
badeend created PR review comment:
Without it, tokio would complain about the future not running within a tokio runtime context. Let me know if there are better ways to deal with this.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ah ok in that case you can handle that with some judicious calls to
with_ambient_tokio_runtime
. This got a panicking test to pass for me for example:diff --git a/crates/wasi/src/preview2/host/tcp.rs b/crates/wasi/src/preview2/host/tcp.rs index 1fce052cd..614495874 100644 --- a/crates/wasi/src/preview2/host/tcp.rs +++ b/crates/wasi/src/preview2/host/tcp.rs @@ -158,7 +160,7 @@ impl<T: WasiView> crate::preview2::host::tcp::tcp::HostTcpSocket for T { TcpState::ConnectReady(result) => result, TcpState::Connecting(mut future) => { let mut cx = std::task::Context::from_waker(futures::task::noop_waker_ref()); - match future.as_mut().poll(&mut cx) { + match with_ambient_tokio_runtime(|| future.as_mut().poll(&mut cx)) { Poll::Ready(result) => result, Poll::Pending => { socket.tcp_state = TcpState::Connecting(future);
alexcrichton commented on PR #7895:
Looking into the
spawn
bits now one orthogonal thing to highlight is that this doesn't compile currently on macOS and there's a few more bits and pieces that need an update (feel free toprtest:full
here)
badeend updated PR #7895.
badeend commented on PR #7895:
Thanks! I've removed the task spawn and fixed the MacOS build.
alexcrichton merged PR #7895.
Last updated: Dec 23 2024 at 12:05 UTC