Wasmtime GitHub notifications bot (Aug 08 2023 at 22:53): Wasmtime GitHub notifications bot (Aug 08 2023 at 22:53):pchickey opened PR #6823 from bytecodealliance:pch/wasi_join_background_tasks to bytecodealliance:main:
This method flushes output and terminates background tasks. Background tasks still terminate as part of Drop!
The problem with the current implementation is that there is no way to wait for output buffered in host background tasks to flush before aborting those tasks as part of dropping the Store/Table. This means that e.g. for a trivial component that prints "hello world\n" to stdout and returns, if the Store&Table drop immediately after execution of the component completes, there is a race and the output may not happen at all.
I don't really love this design, but we got backed into a corner because all of the alternatives we could think up are worse:
- We can't just get rid of the abort-on-drop completely ("daemonize" the tasks) because that means that streams that are connected to e.g. a stalled client connection will consume resources forever, which is not acceptable in some embeddings.
- We can't ensure flushing on drop of a table/store because it requires an await, and rust does not have an async drop
- We can't add an explicit destructor to a table/store which will terminate tasks, and if this destructor is not called tasks will "daemonize", because that means cancellation of the future executing a component before the explicit destructor is called will end up daemonizing the task.
- We could configure all AsyncWriteStreams (and any other stream impls that spawn a task) at creation, or at insertion to the table, with whether they should daemonize on drop or not. This would mean plumbing a bunch of config into places it currently is not.
Therefore, the only behavior we could come up with was to keep the abort-on-drop behavior for background tasks, and add methods to ensure that background tasks are joined (finished) gracefully. This means that both sync and async users of WasiCtx will need to call the appropriate method to wait on background tasks. This is easy enough for users to miss, but we decided that the alternatives are worse.
Closes https://github.com/bytecodealliance/wasmtime/issues/6811
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Aug 08 2023 at 22:53):pchickey requested fitzgen for a review on PR #6823.
Wasmtime GitHub notifications bot (Aug 08 2023 at 22:53):pchickey requested wasmtime-core-reviewers for a review on PR #6823.
Wasmtime GitHub notifications bot (Aug 08 2023 at 22:54):pchickey requested alexcrichton for a review on PR #6823.
Wasmtime GitHub notifications bot (Aug 10 2023 at 23:44):alexcrichton updated PR #6823.
Wasmtime GitHub notifications bot (Aug 10 2023 at 23:59):alexcrichton updated PR #6823.
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev submitted PR review.
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev created PR review comment:
I do wonder if this should also take a timeout just like the sync version. This might encourage users to do the right thing and it has the added benefit of being symetrical with the sync version.
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev created PR review comment:
// In order to make sure output is flushed before dropping, use `flush_output`.
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev created PR review comment:
Nit: is this necessary if you have to check for the pending op anyway down below?
Wasmtime GitHub notifications bot (Aug 11 2023 at 08:44):rylev created PR review comment:
Should this be
sync_flush_output?
Wasmtime GitHub notifications bot (Aug 18 2023 at 00:16):pchickey closed without merge PR #6823.
Last updated: Nov 22 2024 at 16:03 UTC