xia-chao opened PR #14625 from xia-chao:fix/wasi-http-expire-dropped-request to bytecodealliance:main:
p3 had a TODO for a dropped receiver, so wired up
poll_canceled. p2 can't use it, its sync guest blocks the event loop.
xia-chao requested rvolosatovs for a review on PR #14625.
xia-chao requested wasmtime-wasi-reviewers for a review on PR #14625.
xia-chao updated PR #14625.
:memo: rvolosatovs submitted PR review:
Thanks!
I think we could simplify this by dropping the guard and instead relying on
oneshot::Senderfunctionality directly, what do you think?
:speech_balloon: rvolosatovs created PR review comment:
Shouldn't this also wake the worker?
:speech_balloon: rvolosatovs created PR review comment:
in p3 the worker might be processing multiple requests concurrently, returning here would tear down the worker and abort processing requests that are currently in-process and prevent it from accepting more requests, I don't think that's desirable.
I think we should only return here on P2 (which we can do if we always leave the vector empty on P3)
:speech_balloon: rvolosatovs created PR review comment:
We probably don't want this to grow without a bound
We should probably also check
abandonedhere early, since a client could have disconnected while the request was in the queueAdditionally, we should probably check if this is P2 (perhaps by checking
&prepared?) and only push then
:speech_balloon: rvolosatovs created PR review comment:
It seems that we could achieve the same in p2 with:
tx.lock().unwrap().as_mut().is_some_and(|tx| tx.poll_canceled(cx).is_ready())If we stored clones of the
Arc<Mutex<Option<Sender>>>instead of the atomic bool
:speech_balloon: rvolosatovs created PR review comment:
It looks like there is a race condition here if
handlefuture is dropped after the response is received, but before it's polled. Perhaps the simplest fix here could be storing the channel itself in the guard and doingcloseandtry_recvonDrop?
:speech_balloon: rvolosatovs created PR review comment:
Do we actually need the
abandoned? Couldn't we usetxand https://docs.rs/futures/latest/futures/channel/oneshot/struct.Sender.html#method.poll_canceled to check if the receiver has been dropped?
:speech_balloon: rvolosatovs created PR review comment:
let closed = pin!(future::poll_fn(|cx| tx.poll_canceled(cx)));
:speech_balloon: rvolosatovs created PR review comment:
handle, futures::future::select(closed, expiration) ) .await { Either::Left((result, ..)) => (result, true), Either::Right(_) => { // TODO: We should also send a cancel request to the expired // task to give it a chance to shut down gracefully, but as of // this writing Wasmtime does not yet provide an API for doing // that. See issue #11833. Instead, we let it continue running // as a background task until it either returns a response // (which we'll ignore) or the instance itself has expired. (Err(ExpirationError.into()), false) }
:speech_balloon: rvolosatovs created PR review comment:
nit; could we
usethepoll_fnandfuture::imports here?
xia-chao updated PR #14625.
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
I only did 'push only on P2', I don't quite approve of the rest
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
xia-chao updated PR #14625.
:speech_balloon: xia-chao deleted PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
xia-chao updated PR #14625.
:speech_balloon: xia-chao edited PR review comment.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Thank you! Not really agree
:speech_balloon: xia-chao deleted PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
xia-chao updated PR #14625.
:speech_balloon: xia-chao deleted PR review comment.
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
I don't think this needs a change. That drop happens after the response is already in the channel, and I didn't see it get lost.
:speech_balloon: xia-chao deleted PR review comment.
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
I kept the flag. poll_canceled would also see the drop, but this already wakes the outer poll.
:speech_balloon: xia-chao deleted PR review comment.
:memo: xia-chao submitted PR review.
:speech_balloon: xia-chao created PR review comment:
Same idea on p2. The flag and the wake already unblock that wait, so I didn't switch it over.
:speech_balloon: xia-chao edited PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
:speech_balloon: xia-chao deleted PR review comment.
xia-chao requested rvolosatovs for a review on PR #14625.
:repeat: rvolosatovs submitted PR review:
Thanks for the updates, unfortunately it seems that the PR still introduces 2 regressions, one in p2 and one in p3. See https://github.com/rvolosatovs/wasmtime/blob/review/pr-14625-repro/crates/wasi-http/tests/all/pr14625.rs for a reproducer
:repeat: rvolosatovs submitted PR review:
Thanks for the updates, unfortunately it seems that the PR still introduces 2 regressions, one in p2 and one in p3. See https://github.com/rvolosatovs/wasmtime/blob/review/pr-14625-repro/crates/wasi-http/tests/all/pr14625.rs for a reproducer (which passes on
main)
xia-chao requested alexcrichton for a review on PR #14625.
xia-chao requested wasmtime-core-reviewers for a review on PR #14625.
xia-chao updated PR #14625.
xia-chao converted PR #14625 wasi-http: expire requests whose caller dropped the response to a draft.
xia-chao updated PR #14625.
xia-chao has marked PR #14625 as ready for review.
xia-chao requested rvolosatovs for a review on PR #14625.
:speech_balloon: rvolosatovs created PR review comment:
I'm still concerned about this growing without a bound. If that's not an issue, could you add a comment explaining why?
:memo: rvolosatovs submitted PR review:
Thanks for the updates!
See https://github.com/rvolosatovs/wasmtime/commit/fb49791fdae96cbc59c45e092f3d32417877e355
cargo test -p wasmtime-wasi-http --features component-model-async --test all handler
p3_caller_drop_keeps_request_expirationfails on this PR, but passes onmainThis means that once a caller drops, its request's expiration is dropped too: the instance keeps accepting new requests even after
request_timeouthas passed, and the orphaned guest tasks accumulate indefinitely.Could you take a look?
xia-chao commented on PR #14625:
I'm closing this PR.
I also suggest removing the TODO: it conflicts with the new tests,expiring on caller drop is the opposite of leaving the worker accepting.
@rvolosatovs
:cross_mark: xia-chao closed without merge PR #14625.
Last updated: Oct 31 2026 at 23:03 UTC