Stream: git-wasmtime

Topic: wasmtime / PR #6877 WASI preview 2 output-streams: new ba...

Wasmtime GitHub notifications bot (Aug 21 2023 at 23:42):

pchickey opened PR #6877 from bytecodealliance:pch/backpressure_2 to bytecodealliance:main:

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

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Aug 22 2023 at 00:24):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:57):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 00:01):

pchickey updated PR #6877.

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

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 16:38):

pchickey edited PR #6877:

Backpressure design is being revised to take into account that any call with a list<bytes> argument is going to consume the resources of the callee, so the callee has to explicitly permit how much that is allowed to consume.

Flushing is a way to request & observe the completion of a write. Since the write itself is non-blocking it cant report that it failed if that happens after the call is over. the explicit flush call allows the caller to wait for a write (or sequence of writes) to produce an error, or guarantee that it succeeded.

Wasmtime GitHub notifications bot (Aug 23 2023 at 19:20):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 20:12):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 20:40):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 20:43):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 23 2023 at 20:52):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 03:29):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 16:39):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 20:54):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 21:03):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 21:49):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 24 2023 at 23:54):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 00:01):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 00:04):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 00:07):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 07:38):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 07:39):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 16:57):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 18:31):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

This I think wants to be abort-on-drop, and I suspect that should happen elsewhere too, so maybe a AbortOnDropJoinHandle<T> wrapper class?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

s/return Err/bail/

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

Should this have a flush at the end of it as well? Otherwise as-is with this branch a "hello world" on the CLI with components doesn't currently actually print anything (the flush being required to get tokio to wait for the output to actually reach the "actually write to stdio" thread)

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

I think this may not quite work because if the await is cancelled that means the task should be put back in self?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

I think the answer here is a 0-byte read?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

I think the ? here may propagate EWOULDBLOCK to a fatal error?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

Like below, I think this needs to handle the case where the await is cancelled but the task is restored into self

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

Could this constant be abstracted elsewhere for this crate? I saw that tcp yeilds 1GB where this is 64k which is pretty different

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

For this, and below, could the error be threaded through to die_in_worker to be picked up on the next call to flush/write?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

Should this perhaps delegate to self.stream.flush().await?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

This part about all the queueing makes me a bit uneasy. I'd naively expect at-most-one thing here contractually and then we'd add vectors/etc as optimizations in the future maybe.

I think this stems though from the contract of flush/write? I'm personally of the opinion that we should change the WASI layer to only allow at most one operation at a time if we can, meaning that each stream has a single pollable which indicates the status of the pending operation, if any. I know @pchickey you were thinking we should allow concurrent flush/write, though.

Maybe we should jump on a call for a chat about this?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

This raises a few questions to me:

• How is the number of written bytes signaled back?
• The try_write here I think returns EWOULDBLOCK, right?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:01):

alexcrichton created PR review comment:

Could this use one Arc instead of Arc-per-field?

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2023 at 19:04):

alexcrichton created PR review comment:

For reproducing this I've done:

$ cargo build -p wasi-preview1-component-adapter --target wasm32-wasi --features command --no-default-features
$ cat foo.rs
fn main() {
    println!("wat");
}
$ rustc foo.rs --target wasm32-wasi
$ wasm-tools component new ./foo.wasm -o foo.component.wasm --adapt ./target/wasm32-wasi/debug/wasi_snapshot_preview1.wasm
$ cargo run --features component-model -- --wasm-features component-model ./foo.component.wasm

and no output comes out

Wasmtime GitHub notifications bot (Aug 25 2023 at 20:32):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 25 2023 at 20:32):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2023 at 20:32):

pchickey created PR review comment:

Yes, that was the intent. Fixed in 8a91dd

Wasmtime GitHub notifications bot (Aug 25 2023 at 20:32):

pchickey edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2023 at 20:50):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 26 2023 at 03:11):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 28 2023 at 23:47):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 29 2023 at 23:44):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 00:21):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 16:43):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 18:23):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 20:07):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 21:15):

pchickey edited PR #6877:

Backpressure design is being revised to take into account that any call with a list<bytes> argument is going to consume the resources of the callee, so the callee has to explicitly permit how much that is allowed to consume.

Flushing is a way to request & observe the completion of a write. Since the write itself is non-blocking it cant report that it failed if that happens after the call is over. the explicit flush call allows the caller to wait for a write (or sequence of writes) to produce an error, or guarantee that it succeeded.

Fixes #6811

Wasmtime GitHub notifications bot (Aug 30 2023 at 23:05):

pchickey updated PR #6877.

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

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 23:53):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 30 2023 at 23:55):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 30 2023 at 23:55):

elliottt created PR review comment:

This should be resolved after merging in #6928.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:02):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:03):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:03):

elliottt created PR review comment:

Works for me!

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:03):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:03):

elliottt created PR review comment:

This method no longer exists, but that would have good!

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:07):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:07):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2023 at 00:07):

elliottt created PR review comment:

Great point!

Wasmtime GitHub notifications bot (Aug 31 2023 at 16:29):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 17:11):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 17:16):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 17:24):

elliottt updated PR #6877.

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

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 18:21):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 18:51):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 21:41):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 21:43):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 23:35):

pchickey updated PR #6877.

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

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Aug 31 2023 at 23:59):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 00:09):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Syntax-wise, is it possible to write this as s.write_read().await??

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Could 2048 be a documented constant here? That seems a little small to have as a budget, but we can always change it later.

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

This feels a little odd to me in the sense that the trap/context make sense if this is going to be a trap/complete failure but this is also being wrapped up in LastOperationFailed below additionally. My guess, having not read further yet, is that there's error handling of OutputStreamError which makes this not always a trap. This looks like, given the wrapper on this line though, that it's always a trap?

I'll dig in more when seeing the error handling.

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Should this use preview2::spawn to handle the sync case too?

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Question on this, is the intention that the host will consume output from a MemoryOutputBuffer incrementally, freeing up space in the pipe? Or is the intention that everything is collected and then the pipe is completely done and no more data can ever be put in?

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Could the error be handled here to detect EWOULDLBOCK? I'm worried about spurious writable notifications from above causing the write here to immediately return "WOULDBLOCK", but being able to handle that

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton submitted PR review:

This all looks great to me, thanks again so much for pushing on this @pchickey and @elliottt!

I've got a number of minor comments but everything looks great :+1:

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton submitted PR review:

This all looks great to me, thanks again so much for pushing on this @pchickey and @elliottt!

I've got a number of minor comments but everything looks great :+1:

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Could this be:

if let Some(handle) = &mut self.write_handle {
    handle.await...
}

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Like way up a above IMO this should be .unwrap or .expect to propagate a panic rather than hiding it under a trap

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Could this use e.source()? Otherwise errors will get double-printed because the error is printed in Display above and would additionally be printed when the source is printed here

Wasmtime GitHub notifications bot (Sep 01 2023 at 14:23):

alexcrichton created PR review comment:

Oh actually I think I'm totally wrong. I think this is handling the Result layer of the spawned task which is propagating panics.

In that case, could unwrap or expect be used here instead of context/map_err?

Wasmtime GitHub notifications bot (Sep 01 2023 at 15:56):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:08):

pchickey edited PR #6877:

Backpressure design is being revised to take into account that any call with a list<bytes> argument is going to consume the resources of the callee, so the callee has to explicitly permit how much that is allowed to consume.

Flushing is a way to request & observe the completion of a write. Since the write itself is non-blocking it cant report that it failed if that happens after the call is over. the explicit flush call allows the caller to wait for a write (or sequence of writes) to produce an error, or guarantee that it succeeded.

Fixes #6811

This PR changes the wit interfaces, therefore we are leaving it as a draft until after WasmCon & Componentize the World (Sept 6-8) so that wasmtime's main reflects a consistent set of interfaces for demos and hacking at those events.

We plan to merge this PR to main on Monday, Sept 11. We will then backport it as a cherry-pick to the 13.0 release branch so that these changes get included in the wasmtime 13 release as well.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:18):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:18):

pchickey created PR review comment:

yes, this should be an expect instead of a trap.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:25):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:25):

pchickey created PR review comment:

The intention is that writes get collected up to the stated capacity, then no more are accepted.

We could return Err(OutputStreamError::Closed) once capacity is reached, but I wasn't really sure.

We basically only use this type to collect output in tests.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:26):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:26):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:26):

pchickey created PR review comment:

fxied

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:26):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:26):

pchickey created PR review comment:

Thanks, I frequently get that one wrong

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey created PR review comment:

done

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey created PR review comment:

thanks, done

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:55):

pchickey created PR review comment:

i changed AbortOnDropJoinHandle's Future impl to always unwrap the task panics

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:58):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:59):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:59):

pchickey created PR review comment:

I upped it to 4096 because thats what we let blocking-write-and-flush accept

Wasmtime GitHub notifications bot (Sep 01 2023 at 16:59):

pchickey edited PR review comment.

Wasmtime GitHub notifications bot (Sep 01 2023 at 17:19):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 17:37):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 01 2023 at 18:03):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 06 2023 at 21:17):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 08 2023 at 21:33):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 15:56):

pchickey has marked PR #6877 as ready for review.

Wasmtime GitHub notifications bot (Sep 11 2023 at 15:56):

pchickey requested wasmtime-core-reviewers for a review on PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 15:56):

pchickey requested fitzgen for a review on PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 15:56):

pchickey requested wasmtime-default-reviewers for a review on PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 15:57):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 16:16):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 17:06):

pchickey updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 18:33):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 18:34):

pchickey has enabled auto merge for PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:39):

elliottt has disabled auto merge for PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 22:41):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 11 2023 at 23:27):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 15:54):

alexcrichton updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 15:54):

alexcrichton requested wasmtime-fuzz-reviewers for a review on PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 15:54):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 15:57):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 15:57):

alexcrichton updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 16:12):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 16:30):

elliottt updated PR #6877.

Wasmtime GitHub notifications bot (Sep 12 2023 at 18:14):

elliottt merged PR #6877.

Last updated: Oct 23 2024 at 20:03 UTC