badeend opened PR #7749 from badeend:shutdown-close-streams
to bytecodealliance:main
:
Make
tcp-socket::shutdown
close the input/output-streams.
This was already specified in the WITs, but not implemented nor enforced by any tests.
badeend requested fitzgen for a review on PR #7749.
badeend requested wasmtime-core-reviewers for a review on PR #7749.
alexcrichton submitted PR review:
Thanks! One question but otherwise looks good :+1:
alexcrichton submitted PR review:
Thanks! One question but otherwise looks good :+1:
alexcrichton created PR review comment:
Should this be checked after checking for
Waiting
below? It's a bit weird to have a pending write and then call shutdown, but I think theshutdown
syscall will cause the background write to wake up and report its failure?
badeend submitted PR review.
badeend created PR review comment:
The user-facing behavior implemented in this PR seems correct to me. I.e. to return
StreamError::Closed
even if an background write was still in progress.With the current solution it's the responsibility of the caller to properly wait for all data to be fully acknowledged/flushed _before_ calling
shutdown
.This issue is not limited to just
shutdown
, but alsodrop
.I think the true question is: whether or not to abandon the background write (as currently current implemented) or detach the running background tasks and let the background write finish up even after the WASI input stream has been closed (on
drop
orshutdown
).
badeend edited PR review comment.
badeend edited PR review comment.
badeend edited PR review comment.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh I'm not advocating for letting this proceed in the background, I'm more thinking that it might be best to shuffle this to get the response from the background task before checking the shutdown status. I'm under the impression that a
shutdown
will cancel pending writes by marking the socket as writable and returningOk(0)
orEPIPE
or something like that. That would mean that after theshutdown
it's not an indefinite block for the background task to finish up and for us to get the result.
badeend submitted PR review.
badeend created PR review comment:
it might be best to shuffle this to get the response from the background task before checking the shutdown status.
What would be the benefit? On line 296 I reset the last_write status which drops the
AbortOnDropJoinHandle
. So the background task will always be aborted, right?
alexcrichton submitted PR review.
alexcrichton created PR review comment:
It's true yeah that largely the same thing will happen, but I'd at least personally prefer to explicitly clean up the task rather than let it get cleaned up in the background if possible. It's sort of six-to-one-half-dozen-or-the-other though.
I'm not certain of the portability of the behavior of marking the socket as writable and cancelling all writes once
shutdown
is issued though. If that's not the actual OS behavior then switching the order here wouldn't work.Given the discussion here though I'm also a bit wary of cancelling active writes since that could show up as surprising for users, so maybe I should be advocating for letting this proceed in the background...
badeend closed without merge PR #7749.
badeend commented on PR #7749:
Fixed in https://github.com/bytecodealliance/wasmtime/pull/9055 & https://github.com/bytecodealliance/wasmtime/pull/9225
Last updated: Dec 23 2024 at 12:05 UTC