Skip to content

wasi-sockets: (TCP) Use tokio's built in methods to perform the state changes#7895

Merged
alexcrichton merged 4 commits intobytecodealliance:mainfrom
badeend:tcp-tokio
Feb 12, 2024
Merged

wasi-sockets: (TCP) Use tokio's built in methods to perform the state changes#7895
alexcrichton merged 4 commits intobytecodealliance:mainfrom
badeend:tcp-tokio

Conversation

@badeend
Copy link
Member

@badeend badeend commented Feb 8, 2024

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 call epoll_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 badeend requested a review from a team as a code owner February 8, 2024 13:15
@badeend badeend requested review from alexcrichton and removed request for a team February 8, 2024 13:15
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Feb 8, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! One very small comment but otherwise everything looks as I'd expect here 👍

pub fn tcp_socket(&self) -> &tokio::net::TcpStream {
&self.inner
pub(crate) fn connect(socket: tokio::net::TcpSocket, addr: std::net::SocketAddr) -> impl Future<Output = io::Result<tokio::net::TcpStream>> + Send {
crate::preview2::spawn(socket.connect(addr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this does a spawn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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 to prtest:full here)

@badeend
Copy link
Member Author

badeend commented Feb 11, 2024

Thanks! I've removed the task spawn and fixed the MacOS build.

@alexcrichton alexcrichton added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bytecodealliance:main with commit 7b4fb8c Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasi Issues pertaining to WASI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants