Stream: git-wasmtime

Topic: wasmtime / PR #7749 Make `shutdown` close the input/outpu...

Wasmtime GitHub notifications bot (Jan 04 2024 at 18:22):

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.

Wasmtime GitHub notifications bot (Jan 04 2024 at 18:22):

badeend requested fitzgen for a review on PR #7749.

Wasmtime GitHub notifications bot (Jan 04 2024 at 18:22):

badeend requested wasmtime-core-reviewers for a review on PR #7749.

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:41):

alexcrichton submitted PR review:

Thanks! One question but otherwise looks good :+1:

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:41):

alexcrichton submitted PR review:

Thanks! One question but otherwise looks good :+1:

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:41):

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 the shutdown syscall will cause the background write to wake up and report its failure?

Wasmtime GitHub notifications bot (Jan 06 2024 at 13:36):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Jan 06 2024 at 13:36):

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 also drop.

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 or shutdown).

Wasmtime GitHub notifications bot (Jan 06 2024 at 13:37):

badeend edited PR review comment.

Wasmtime GitHub notifications bot (Jan 06 2024 at 13:38):

badeend edited PR review comment.

Wasmtime GitHub notifications bot (Jan 06 2024 at 13:41):

badeend edited PR review comment.

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:14):

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 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.

Wasmtime GitHub notifications bot (Jan 08 2024 at 19:39):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 19:39):

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?

Wasmtime GitHub notifications bot (Jan 09 2024 at 15:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 09 2024 at 15:20):

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...

Wasmtime GitHub notifications bot (Sep 12 2024 at 17:54):

badeend closed without merge PR #7749.

Wasmtime GitHub notifications bot (Sep 12 2024 at 17:55):

badeend commented on PR #7749:

Fixed in https://github.com/bytecodealliance/wasmtime/pull/9055 & https://github.com/bytecodealliance/wasmtime/pull/9225

Last updated: Nov 22 2024 at 16:03 UTC