Skip to content

Commit

Permalink
Fix network tests in synchronous mode
Browse files Browse the repository at this point in the history
This commit is an attempt to fix some networking tests in synchronous
mode in our test suite. Currently networking tests don't actually run in
synchronous mode on CI which is why no failures have been surfaced yet,
but the refactoring in bytecodealliance#7182 is going to start doing this.

Currently the `udp_sample_application.rs` test blocks infinitely in
synchronous mode for me locally, most of the time. This appears to be an
interaction between how Tokio handles readiness and how we're
entering the event loop. We're effectively entering the Tokio event loop
with a future that's always ready which ends up starving Tokio of
otherwise performing its background work such as updating flags for
readiness of reading/writing.

The fix here is to add a yield at the start of an `in_tokio` block which
is used in synchronous mode. This is a kludge fix but the intention is
to enable Tokio to have a chance to update readiness flags and process
events from epoll/kqueue/etc.

An additional fix to this issue is WebAssembly/wasi-sockets#64 where the
test is waiting on `READABLE` or `WRITABLE`, but in this specific case
it should only wait on `READABLE`. If it waited on just this then that
would also fix this issue. Nevertheless having a `yield_now` is expected
to have little-to-no overhead and otherwise fix this edge case of an
always-ready future.
  • Loading branch information
alexcrichton committed Oct 9, 2023
1 parent 36d4afc commit dea65f7
Showing 1 changed file with 41 additions and 1 deletion.
42 changes: 41 additions & 1 deletion crates/wasi/src/preview2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,49 @@ pub fn in_tokio<F: Future>(f: F) -> F::Output {
let _enter = h.enter();
h.block_on(f)
}
// The `yield_now` here is non-obvious and if you're reading this
// you're likely curious about why it's here. This is currently required
// to get some features of "sync mode" working correctly, such as with
// the CLI. To illustrate why this is required, consider a program
// organized as:
//
// * A program has a `pollable` that it's waiting on.
// * This `pollable` is always ready .
// * Actually making the corresponding operation ready, however,
// requires some background work on Tokio's part.
// * The program is looping on "wait for readiness" coupled with
// performing the operation.
//
// In this situation this program ends up infinitely looping in waiting
// for pollables. The reason appears to be that when we enter the tokio
// runtime here it doesn't necessary yield to background work because
// the provided future `f` is ready immediately. The future `f` will run
// through the list of pollables and determine one of them is ready.
//
// Historically this happened with UDP sockets. A test send a datagram
// from one socket to another and the other socket infinitely didn't
// receive the data. This appeared to be because the server socket was
// waiting on `READABLE | WRITABLE` (which is itself a bug but ignore
// that) and the socket was currently in the "writable" state but never
// ended up receiving a notification for the "readable" state. Moving
// the socket to "readable" would require Tokio to perform some
// background work via epoll/kqueue/handle events but if the future
// provided here is always ready, then that never happened.
//
// Thus the `yield_now()` is an attempt to force Tokio to go do some
// background work eventually and look at new interest masks for
// example. This is a bit of a kludge but everything's already a bit
// wonky in synchronous mode anyway. Note that this is hypothesized to
// not be an issue in async mode because async mode typically has the
// Tokio runtime in a separate thread or otherwise participating in a
// larger application, it's only here in synchronous mode where we
// effectively own the runtime that we need some special care.
Err(_) => {
let _enter = RUNTIME.enter();
RUNTIME.block_on(f)
RUNTIME.block_on(async move {
tokio::task::yield_now().await;
f.await
})
}
}
}
Expand Down

0 comments on commit dea65f7

Please sign in to comment.