Make streams owned by request/response that they are tied to. #6228

brendandburns · 2023-04-18T04:15:16Z

This is related to the discussion here: WebAssembly/WASI#818

Where we decided that streams should be child-own properties 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.

brendandburns · 2023-04-18T04:15:32Z

cc @pchickey

brendandburns · 2023-04-18T15:22:19Z

fwiw, looks like tests failed b/c of a network failure pulling the wasi-http submodule.

pchickey · 2023-04-18T17:54:10Z

I hit rerun and now it got to a test failure

alexcrichton · 2023-04-18T18:28:53Z

I'm going to move this to @pchickey's review queue as he's more familiar with the implementation than I

rvolosatovs · 2023-04-19T06:19:59Z

crates/wasi-http/src/types_impl.rs

    }
    fn drop_outgoing_request(&mut self, request: OutgoingRequest) -> wasmtime::Result<()> {
-        self.requests.remove(&request);
+        match self.requests.get(&request) {


nit; since we only care about the Some case, how about an if 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

Done.

Thanks for the suggestion, I'm still learning idiomatic rust)

rvolosatovs · 2023-04-19T06:30:22Z

crates/wasi-http/src/struct.rs

 use bytes::Bytes;
 use std::collections::HashMap;

+#[derive(Clone)]


How about a derive(Default)?

rvolosatovs · 2023-04-19T06:31:28Z

crates/wasi-http/src/struct.rs


+impl Stream {
+    pub fn new() -> Self {
+        Self {


If we had a Default trait impl, this could have just called Self::default()

rvolosatovs · 2023-04-19T06:32:31Z

crates/wasi-http/src/streams_impl.rs

+        let st = self
+            .streams
+            .get_mut(&stream)
+            .ok_or_else(|| anyhow!("stream not found: {stream}"))?;


Doesn't with_context work here?

I'm not sure I'm following your suggestion, can you clarify?

Sure, I'm referring to this method on the anyhow::Context trait https://docs.rs/anyhow/latest/anyhow/trait.Context.html#tymethod.with_context

If it's in scope, you should be able to call context or with_context (usually used with format! and similar) on most Result and all Option types directly, thus eliminating the need for ok_or_else and explicit anyhow! invocation

@rvolosatovs I made those ok_or_else(|| anyhow!(...)) suggestions in a prior PR and wasn't aware that Context could make that more succinct. 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 inserting tracing statements which someone debugging can recover the values of the arguments from.

rvolosatovs · 2023-04-19T06:35:22Z

crates/wasi-http/src/streams_impl.rs

+                    this,
+                    Stream {
+                        closed: false,
+                        data: bytes::Bytes::from(buf.clone()),


You wouldn't need this clone if the call to buf.len below was moved higher, before the match.

rvolosatovs · 2023-04-19T06:36:51Z

crates/wasi-http/src/http_impl.rs

            self.streams
                .get(&request.body)
-                .unwrap_or(&Bytes::new())
+                .unwrap_or(&Stream::new())


If we had Default implemented, this could have been unwrap_or_default

Unfortunately, HashMap::get(...) returns a reference rather than a value, and there's no default for a reference. I could implement the Default trait for &Stream references, but I'm not sure if that's what is best. Let me know.

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

rvolosatovs · 2023-04-19T06:38:03Z

crates/wasi-http/src/struct.rs

 use std::collections::HashMap;

+#[derive(Clone)]
+pub struct Stream {


Looking at the usages of this struct, it seems that a From<Bytes> implementation could really clear this up

rvolosatovs · 2023-04-19T06:38:56Z

crates/wasi-http/src/streams_impl.rs

-                self.streams.insert(this, new.freeze());
+                self.streams.insert(
+                    this,
+                    Stream {


If there was a From implementation, this could have been Stream::from(new.freeze())

brendandburns · 2023-04-19T22:12:01Z

Comments generally addressed, couple where I need some more feedback.

Please take another look.

Thanks!

rvolosatovs

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?

rvolosatovs · 2023-04-19T22:37:54Z

crates/wasi-http/src/http_impl.rs

-        self.streams.insert(response.body, buf.freeze());
+        self.streams.insert(
+            response.body,
+            Stream {


This should probably also use the From trait

brendandburns · 2023-04-20T17:00:31Z

I'm not sure I follow your question. Are you asking why we don't use BytesMut instead of a Bytes in the Stream struct? We could do that if we wanted to, I'm not sure it actually matters that much because we'd need to turn it back into a single contiguous array of bytes at some point, but it might be more efficient.

Let me know if that's the change you are suggesting, or if I'm missing something.

Thanks!

crates/wasi-http/src/streams_impl.rs

pchickey · 2023-04-20T18:07:54Z

@brendandburns I can merge this when ready, please ping when the above discussion is resolved.

rvolosatovs · 2023-04-20T18:09:15Z

crates/wasi-http/src/streams_impl.rs

+                    bail!("stream is dropped!");
+                }
+                let new_len = st.data.len() + len;
+                let mut new = bytes::BytesMut::with_capacity(new_len);


From #6228 (comment), as you said, I think I'd just use BytesMut and https://docs.rs/bytes/latest/bytes/struct.BytesMut.html#method.extend_from_slice here

Since 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)

Done. I needed to use .or_default().extend_from_slice(...) but I think it achieves the same goal (and thanks for the suggestion of implementing Default that made it possible :)

Co-authored-by: Pat Hickey <[email protected]>

brendandburns · 2023-04-20T23:00:04Z

@pchickey I think this is ready to merge.

…dealliance#6228) * Make streams owned by request/response that they are tied to. * Address comments, fix tests. * Address comment. * Update crates/wasi-http/src/streams_impl.rs Co-authored-by: Pat Hickey <[email protected]> * Switch to BytesMut --------- Co-authored-by: Pat Hickey <[email protected]>

Make streams owned by request/response that they are tied to.

c7b3f25

brendandburns requested a review from a team as a code owner April 18, 2023 04:15

brendandburns requested review from alexcrichton and removed request for a team April 18, 2023 04:15

pchickey mentioned this pull request Apr 18, 2023

feat: align wasi-http with component linker #6195

Merged

alexcrichton requested review from pchickey and removed request for alexcrichton April 18, 2023 18:29

rvolosatovs reviewed Apr 19, 2023

View reviewed changes

Address comments, fix tests.

a72d8e0

brendandburns force-pushed the ptr branch from 7dd25d2 to a72d8e0 Compare April 19, 2023 22:12

rvolosatovs approved these changes Apr 19, 2023

View reviewed changes

Address comment.

14b25d2

pchickey approved these changes Apr 20, 2023

View reviewed changes

crates/wasi-http/src/streams_impl.rs Outdated Show resolved Hide resolved

pchickey mentioned this pull request Apr 20, 2023

Initial implementation of wasi-http serving #6091

Closed

rvolosatovs approved these changes Apr 20, 2023

View reviewed changes

brendandburns and others added 3 commits April 20, 2023 12:52

Update crates/wasi-http/src/streams_impl.rs

22a2dd5

Co-authored-by: Pat Hickey <[email protected]>

Switch to BytesMut

bf2c3ba

Merge branch 'ptr' of https://github.com/brendandburns/wasmtime into ptr

b855a4b

pchickey added this pull request to the merge queue Apr 21, 2023

Merged via the queue into bytecodealliance:main with commit 43ec481 Apr 21, 2023

rvolosatovs mentioned this pull request Apr 24, 2023

feat(http): allow custom OutgoingHandler implementations #6272

Closed

Make streams owned by request/response that they are tied to. #6228

Make streams owned by request/response that they are tied to. #6228

Uh oh!

Conversation

brendandburns commented Apr 18, 2023

Uh oh!

brendandburns commented Apr 18, 2023

Uh oh!

brendandburns commented Apr 18, 2023

Uh oh!

pchickey commented Apr 18, 2023

Uh oh!

alexcrichton commented Apr 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchickey Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 19, 2023

Uh oh!

rvolosatovs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 20, 2023

Uh oh!

Uh oh!

pchickey commented Apr 20, 2023

Uh oh!

rvolosatovs Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

pchickey Apr 20, 2023 •

edited

Loading

rvolosatovs Apr 20, 2023 •

edited

Loading