Make shutdown close the input/output-streams.
#7749
Conversation
This was already specified in the WITs, but not enforced by any tests.
| if self.inner.is_shut_down_for_sending() { | ||
| self.last_write = LastWrite::Done; | ||
| return Err(StreamError::Closed); | ||
| } |
There was a problem hiding this 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 the shutdown syscall will cause the background write to wake up and report its failure?
There was a problem hiding this 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.
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 output stream has been closed. Because this issue is not limited to just shutdown, but also drop.
There was a problem hiding this 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 returning Ok(0) or EPIPE or something like that. That would mean that after the shutdown it's not an indefinite block for the background task to finish up and for us to get the result.
There was a problem hiding this 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?
There was a problem hiding this 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...
Make
tcp-socket::shutdownclose the input/output-streams.This was already specified in the WITs, but not implemented nor enforced by any tests.