Wasmtime GitHub notifications bot (Apr 18 2023 at 04:15): Wasmtime GitHub notifications bot (Apr 18 2023 at 04:15):brendandburns opened PR #6228 from brendandburns:ptr to bytecodealliance:main:
This is related to the discussion here: https://github.com/WebAssembly/wasi-http/issues/24
Where we decided that streams should be
child-ownproperties of the request or response that they are tied to.This adds clarity to when it is valid to drop an input or output stream and in what order relative to the response.
Wasmtime GitHub notifications bot (Apr 18 2023 at 04:15):brendandburns requested alexcrichton for a review on PR #6228.
Wasmtime GitHub notifications bot (Apr 18 2023 at 04:15):brendandburns requested wasmtime-core-reviewers for a review on PR #6228.
Wasmtime GitHub notifications bot (Apr 18 2023 at 18:29):alexcrichton requested pchickey for a review on PR #6228.
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
Doesn't
with_contextwork here?
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
nit; since we only care about the
Somecase, how about anif let Some(r) = self.requests.get(&request)instead?Or even
if let Entry::Occupied(e) = self.requests.entry(request) { let r = e.remove();https://doc.rust-lang.org/std/collections/hash_map/struct.OccupiedEntry.html#method.remove
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
If we had a
Defaulttrait impl, this could have just calledSelf::default()
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
How about a
derive(Default)?
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
You wouldn't need this clone if the call to
buf.lenbelow was moved higher, before the match.
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
If we had
Defaultimplemented, this could have beenunwrap_or_default
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
Looking at the usages of this struct, it seems that a
From<Bytes>implementation could really clear this up
Wasmtime GitHub notifications bot (Apr 19 2023 at 06:39):rvolosatovs created PR review comment:
If there was a
Fromimplementation, this could have beenStream::from(new.freeze())
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:52):brendandburns created PR review comment:
Unfortunately,
HashMap::get(...)returns a reference rather than a value, and there's no default for a reference. I could implement theDefaulttrait for&Streamreferences, but I'm not sure if that's what is best. Let me know.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:52):brendandburns created PR review comment:
done.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:52):brendandburns created PR review comment:
done.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:53):brendandburns created PR review comment:
I'm not sure I'm following your suggestion, can you clarify?
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:53):brendandburns created PR review comment:
done.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:53):brendandburns created PR review comment:
added.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:53):brendandburns created PR review comment:
done.
Wasmtime GitHub notifications bot (Apr 19 2023 at 21:54):brendandburns created PR review comment:
Done.
Thanks for the suggestion, I'm still learning idiomatic rust)
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:11):brendandburns updated PR #6228.
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:12):brendandburns updated PR #6228.
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:22):rvolosatovs created PR review comment:
Sure, I'm referring to this method on the
anyhow::Contexttrait https://docs.rs/anyhow/latest/anyhow/trait.Context.html#tymethod.with_contextIf it's in scope, you should be able to call
contextorwith_context(usually used withformat!and similar) on mostResultand allOptiontypes directly, thus eliminating the need forok_or_elseand explicitanyhow!invocation
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:35):rvolosatovs created PR review comment:
That makes sense, I think this is fine.
I am making a few changes here to allow for the host to provide a custom connect implementation, so I'll take a deeper look at this as well next week
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:40):rvolosatovs submitted PR review:
Just a general question, why are we not modifying the stream data buffer, but rather cloning it? In other words, why would we ever want to read the same bytes more than once?
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:40):rvolosatovs submitted PR review:
Just a general question, why are we not modifying the stream data buffer, but rather cloning it? In other words, why would we ever want to read the same bytes more than once?
Wasmtime GitHub notifications bot (Apr 19 2023 at 22:40):rvolosatovs created PR review comment:
This should probably also use the
Fromtrait
Wasmtime GitHub notifications bot (Apr 20 2023 at 16:57):brendandburns created PR review comment:
done.
Wasmtime GitHub notifications bot (Apr 20 2023 at 17:57):pchickey created PR review comment:
I made those ok_or_else(|| anyhow!(...)) suggestions in a prior PR and wasn't aware that Context could do that for me. Thank you.
@brendandburns The suggested change here is
let st = self.streams.get_mut(&stream).with_context(|| format!("stream not found: {stream}"))?;, or maybe even....get_mut(&stream).context("stream not found")?if leaving out the stream number is ok - and it probably is, because (at least when using the component linker, I'm not sure if it works in module mode) bindgen is insertingtracingstatements which someone debugging can recover the values of the arguments from.
Wasmtime GitHub notifications bot (Apr 20 2023 at 17:58):pchickey edited PR review comment.
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:01):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:01):pchickey submitted PR review.
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:01):pchickey created PR review comment:
bail!("cannot write to closed stream");
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):rvolosatovs submitted PR review.
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):rvolosatovs created PR review comment:
From https://github.com/bytecodealliance/wasmtime/pull/6228#issuecomment-1516661764, as you said, I think I'd just use
BytesMutand https://docs.rs/bytes/latest/bytes/struct.BytesMut.html#method.extend_from_slice hereThis that would be quite a bit shorter, it could probably also make sense to remove the match altogether and instead rely on https://doc.rust-lang.org/std/collections/hash_map/enum.Entry.html#method.and_modify (see example there with
or_insert)
Wasmtime GitHub notifications bot (Apr 20 2023 at 18:10):rvolosatovs edited PR review comment.
Wasmtime GitHub notifications bot (Apr 20 2023 at 19:53):brendandburns updated PR #6228.
Wasmtime GitHub notifications bot (Apr 20 2023 at 22:59):brendandburns updated PR #6228.
Wasmtime GitHub notifications bot (Apr 20 2023 at 22:59):brendandburns created PR review comment:
Done. I needed to use
.or_default().extend_from_slice(...)but I think it achieves the same goal (and thanks for the suggestion of implementingDefaultthat made it possible :)
Wasmtime GitHub notifications bot (Apr 21 2023 at 18:57):pchickey merged PR #6228.
Wasmtime GitHub notifications bot (Apr 24 2023 at 14:44):rvolosatovs created PR review comment:
Last updated: Dec 23 2024 at 13:07 UTC