wasi-http: p3 single-instance-per-request coverage and fixes#13345
wasi-http: p3 single-instance-per-request coverage and fixes#13345posborne wants to merge 1 commit into
Conversation
|
Thanks! Could a test be added for this as well? For example |
Yeah, I'll add a test for this general case (one request per session) -- the change here just removes error logging in the non-error case but coverage would be useful even if it isn't asserting on log non-existence. |
|
@alexcrichton In doing these tests, I think I found a related bug that breaks use of the future returned by Currently, in wasmtime, it is possible for this to indicate completion before the bytes have been handed off to hyper. If the guest keys off this to exit, then bytes are dropped on the floor. Here I have pushed 3 additional commits:
The final patch in the series (dfe3349) I'm not entirely particularly confident in as I don't feel I have a confident grasp on the various futures going on and where/how errors in this chain might be involved. @tschneidereit It sounds like you've also been doing some work with |
posborne
changed the title
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Everything here looks reasonable to me, but I also get tangled up in the futures here. CI looks like it's failing also on a channel/future-related thing which seems like it might be either a preexisting race or one accidentally introduced here?
| // Resolve the guest's `tx_result` future (returned by `response.new`) | ||
| // when hyper has finished reading the response body, not at | ||
| // request-resource cleanup time. Previously `request_io_result` was | ||
| // passed here, which resolved `tx_result` before the body had been | ||
| // handed off to hyper. A guest that calls `exit(ok)` after awaiting | ||
| // `tx_result` would therefore tear down the store while the body was | ||
| // still in flight. |
There was a problem hiding this comment.
It's ok to drop the mention of the old code here, no need to document what-once-was and I think the first part of these docs stand well on their own
I'm working on reproducing this; it's similar to some of the kind of races I saw in the existing code which I had hoped had been addressed but some more digging is clearly warranted. |
|
@alexcrichton I think it's ready to go, was just waiting on CI yesterday -- I did get a local repro so was fairly confident in the patch. I can do the rebase and put it into the merge queue when ready. |
When a guest calls wasi:cli/exit(ok) from within a wasi:http/handler instance, the host exit implementation returns Err(I32Exit(0)). This propagated as a worker error, causing wasmtime serve to log an error and close queued request channels rather than creating fresh instances. I32Exit(0) semantically means 'this instance is done cleanly'. Treat it as a graceful worker exit rather than an error in the Worker::run path, allowing guests to opt out of instance reuse via the exit+backpressure pattern described in WebAssembly/WASI#898. In addition to not logging an error, this patch also changes the signal used to indicate that the response has been transmitted (as returned to the guest by response.new) to be tied to the lifetime of the response. This avoids races that were previously present on the handoff of responses to the http stack and notifications to the guest of response transmission (which caused issues when exiting from this signal). Tests are added related to the single-request-per-instance pattern that had previously failed and was racy.
posborne
force-pushed
the
wasi-http-cli-exit-0-not-error
branch
from
ee9bafc to
97819d9
Compare
2 participants
When a guest calls
wasi:cli/exit(ok)from within awasi:http/handlerinstance, the host exit implementation returns Err(I32Exit(0)). This propagated as a worker error, causing wasmtime serve to log a spurious "worker error" message and wasm backtrace on what is actually a clean, guest-controlled shutdown.