wasmtime / issue #6823 add a join_background_tasks method... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #6823 add a join_background_tasks method...

Wasmtime GitHub notifications bot (Aug 08 2023 at 22:54):

Waiting to merge until @elliottt and I add a test to show that waiting for join_background_tasks flushes all output.

Wasmtime GitHub notifications bot (Aug 08 2023 at 22:56):

pchickey edited a comment on issue #6823:

Waiting to merge until

@elliottt and I add a test to show that waiting for join_background_tasks flushes all output.

all uses of wasi in tests and examples use this new method to wait for output to flush, so that when people copy paste existing code they dont miss this important detail

Wasmtime GitHub notifications bot (Aug 09 2023 at 06:59):

rylev commented on issue #6823:

I don't see a discussion of the alternative approach where background tasks are daemonized by default but there is a method that returns a future which completes when all background tasks are done or cancels the background tasks on Drop. This would allow for opting into cancelation (say after a timeout) rather than opting out of cancelation (which is easy to forget to do).

Such an approach seems like the ideal approach since it doesn't risk I/O operations not flushing and easily allows users to opt into ensuring that resources are not leaked. If this is really just a question of which default is better (best effort of I/O flushing vs protecting against potential resource leaks), I would personally pick I/O flushing. The approach in this PR seems like it will be a footgun that a lot of users will run into even if the docs are all updated to mention it.

Wasmtime GitHub notifications bot (Aug 09 2023 at 18:26):

alexcrichton commented on issue #6823:

Oh that's discussed a bit in @pchickey's points above, but I can to help clarify some more. I do realize though that yesterday in our sync we ended up concluding daemonize-by-default was the right option, and both @pchickey and I feel the same way but the code ended up not working out too well. One of the main problems with that is that for embeddings where cancel-by-default is desired then that option needs to be active for the entire lifetime of WasiCtx since cancellation may not only happen at the end when tasks are being joined but also at any time during the execution of a guest. This means that the option of daemonize-or-cancel on drop is an up-front configuration option rather than a when-you-make-the-join-future option.

One idea would be to make this an option on WasiCtxBuilder but that required a degree of configuration plumbing which we basically weren't quite ready for. (not that it's impossible I think though). One part about daemonize-by-default though is that I'm not sure how that would work in the sync case because it's not clear to me when the Wasmtime CLI, for example, would perform the "wait" for background tasks to finish before the CLI exits the process.

Given all that the conclusion we reached was to go the opposite way and cancel-on-drop rather than daemonize and require these methods to be invoked to ensure output reaches its destination. I don't disagree it's a footgun though but I think that fixing that may need to happen at a different level rather than daemonize-or-cancel perhaps. One example there was what I mentioned yesterday where blocking-write does something different than what it does today, but as we talked about it's not clear what this would be exactly and whether it would actually work.

Wasmtime GitHub notifications bot (Aug 10 2023 at 13:06):

rylev commented on issue #6823:

That's fine. I do think it would go a long way to make blocking writes only return once wasmtime has handed them off to the underlying host kernel.

I believe we discussed the possibility of adding a call to ready before this line to ensure we don't exit blocking_write until we're sure that the previous write has gone all the way through. Would that be possible?

Wasmtime GitHub notifications bot (Aug 10 2023 at 21:33):

alexcrichton commented on issue #6823:

@pchickey I think there's one more block to handle as well (same as input streams)

Wasmtime GitHub notifications bot (Aug 10 2023 at 21:47):

pchickey commented on issue #6823:

I believe we discussed the possibility of adding a call to ready before this line to ensure we don't exit blocking_write until we're sure that the previous write has gone all the way through. Would that be possible?

That would make blocking write into blocking write + flush, but only work for the one implementation today where ready also means flush. I don't think we want to make every blocking write include a flush, since the flush could hang if the other end of the pipe stops listening, and I also don't want to make ready mean flush on all writers, which limits the flexibility for future implementations.

Wasmtime GitHub notifications bot (Aug 10 2023 at 23:48):

alexcrichton commented on issue #6823:

My previous comment was incorrect as that's related to input streams, not output streams, which is what this cares about.

I've additionally pushed up some more commits after some more discussion with @pchickey. The thinking is that it's pretty likely at this point for WASI to gain a "flush" on output-stream so we may as well start heading in that direction. Furthermore in local testing I found that the join_background_tasks as-is before my commits wasn't actually sufficient, we needed to literally call .flush() on the output stream before returning from join_background_tasks anyway.

So I've updated HostOutputStream to have a flush() method and updated it for the AsyncWriteStream adapter. Additionally I've removed join_background_tasks from HostInputStream and renamed the WasiCtx method to flush_output. This way it's now clear that the final operation to do is to flush all output.

The thinking afterwards, to solve your concern @rylev, is that WASI will gain flush as an inherent method on all output streams. That'll get wired up to the flush I just wrote, and additionally libraries like wasi-libc would be updated to flush at appropriate points. Furthermore the preview1-to-preview2 adapter will probably flush after all writes to stdout since preview1 has no notion of "flushing", which would doubly fix the issue you're seeing @rylev

Wasmtime GitHub notifications bot (Aug 18 2023 at 00:15):

pchickey commented on issue #6823:

I know that various bca people are on the same page but for onlookers: this is just too sticky of an issue to hack on a fix in the host like this, and we identified other issues with output-stream, so we are going to redesign the way output-stream does backpressure to make this no longer be a problem.

Last updated: Nov 22 2024 at 16:03 UTC