wasmtime / PR #14633 wasi-http: Close connections on Stor... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14633 wasi-http: Close connections on Stor...

Wasmtime GitHub notifications bot (Oct 09 2026 at 18:36):

adamrk opened PR #14633 from adamrk:abk/http-tokio-task-ownership to bytecodealliance:main:

Cancel tokio tasked spawned by wasi-http after the corresponding Store is dropped (with a 10 second configurable grace period). Otherwise tokio would keep handling these connections indefinitely in the background with no way to stop them and reclaim the resources.

Wasmtime GitHub notifications bot (Oct 09 2026 at 18:36):

adamrk requested rvolosatovs for a review on PR #14633.

Wasmtime GitHub notifications bot (Oct 09 2026 at 18:36):

adamrk requested wasmtime-wasi-reviewers for a review on PR #14633.

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:memo: rvolosatovs submitted PR review:

Thanks for working on this! Making sure tasks don't outlive their Store makes sense to me, but I think the current structure is heavier than it needs to be. Each request now goes through wasmtime_wasi::runtime::AbortOnDropJoinHandle, DelayedAbortOnDropHandle, a oneshot, a timer task that is always spawned, and a Store-spawned future, plus the Arc<AbortOnDropHandle> references.

Could we use plain tokio types instead, and spawn the timer only when the Store is actually dropped while the task is still running? Something like (LLM-generated):
struct StoreDropGuard {
    abort: AbortHandle,
    grace: Duration,
    rt: tokio::runtime::Handle,
}

impl Drop for StoreDropGuard {
    fn drop(&mut self) {
        if self.abort.is_finished() {
            return;
        }
        if self.grace.is_zero() {
            self.abort.abort();
            return;
        }
        let abort = self.abort.clone();
        let grace = self.grace;
        self.rt.spawn(async move {
            tokio::time::sleep(grace).await;
            abort.abort();
        });
    }
}

let task = tokio::task::spawn(async move { /* … */ });
let abort = task.abort_handle();
let io = Arc::new(AbortOnDropHandle(abort.clone()));
let guard = StoreDropGuard {
    abort: abort.clone(),
    grace,
    rt: tokio::runtime::Handle::current(),
};
store
    .spawn(async move |_| {
        let _guard = guard;
        if let Err(e) = task.await {
            if !e.is_cancelled() {
                debug!(?e, "`send_request` I/O task panicked");
            }
        }
        Ok(())
    })
    .map_err(|e| {
        abort.abort();
        HttpError::trap(e)
    })?;
This would:

remove the timer task and oneshot that are currently created for every request,

avoid the panic on cancellation, since we never poll the WASI AbortOnDropJoinHandle wrapper,

abort immediately if store.spawn fails,

capture the runtime Handle up front, since the Store may be dropped outside of a Tokio context.

This still does not address the issue with guest forwarding the body back to the host, though, so it seems like we have to add a way to opt-out of this mechanism for that particular case.

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

I think that plain tokio::spawn was a better fit here, since it avoids the panic

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

I don't think this is necessary, since this code would always run in Tokio anyway

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

This is probably too long for a unit test, could we shorten the outer timeout and use the set_spawned_task_shutdown_grace_period with something smaller than 10s ?

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

this may panic if the task is cancelled, because of this line https://github.com/bytecodealliance/wasmtime/blob/3d5851be67f079041331b27f492ed6f561c7976e/crates/wasi/src/runtime.rs#L81

We should probably await the inner handle here instead

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

if store.spawn fails, then the task keeps running for shutdown_timeout, since it's already spawned at L243, which is probably not what we want.

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

could we get that already on L171 avoiding a repeated store.with?

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

This would crash the whole store, meaning all instances using this store would be aborted - I don't think that's what we want.

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

This does not work for proxy components where the guest forwards the response body to the host unchanged. In that case the body must outlive the store.
AFAIK being able to efficiently hand over an HTTP body to the host is a desirable feature.

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

            // will all hold references to an abort handle on the task

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

            // not retain ownership of the task, so they cannot keep it alive

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

In case where the host constructs the request and this task is aborted by the timeout, this line is never reached and the receiver gets DROPPED_FUTURE_ERROR, is that intended?

Wasmtime GitHub notifications bot (Oct 13 2026 at 18:51):

:speech_balloon: rvolosatovs created PR review comment:

nit; are these renames required to make this work? if not, could we avoid them to make the diff simpler to review?

Wasmtime GitHub notifications bot (Oct 15 2026 at 14:49):

adamrk commented on PR #14633:

Closing in favor of https://github.com/bytecodealliance/wasmtime/pull/14743

Wasmtime GitHub notifications bot (Oct 15 2026 at 14:49):

:cross_mark: adamrk closed without merge PR #14633.

Last updated: Oct 31 2026 at 23:03 UTC