Implement Response.clone (#125) #178

zerotri · 2024-11-27T11:30:36Z

This simple commit takes the existing Request.clone implementation and adapts it to work for Response.clone. This was done largely as a learning exercise.

I did have a question regarding the current Response class. It appears that a URL slot is missing from Response where it appears in Request. The spec shows a url attribute which would imply that this slot should be implemented.

Is this intentionally left out or something that just hasn't been added yet?

I have yet to get the WPT tests running properly on my machine, so this has not been tested against those. As such, I am leaving this as a draft until I can validate against those.

* Implement `Response.clone` This is largely based on the existing `Request.clone` implementation * Add tests for `Response` to match `Request` tests

guybedford · 2024-11-27T22:14:06Z

This is amazing, thank you for sharing. If you are able to get the tests running it would be great to get the WPT coverage working here.

In order to get the test coverage updated, we run the tests with WPT_FLAGS="--update-expectations". It does seem like some WPT cases have switched from passing to failing in the process as well - it seems like this is actually as a side effect of adding the feature rather than being an explicit regression, so assuming that is the case that should be okay, but it would be worth confirming this first.

With regards to your question, RequestOrResponse is the base class for both which has a URL slot, then the extended slots for Response pick up at this RequestOrResponse::Slots::count so URL is very much still on Response, implemented via its accessor (https://github.com/bytecodealliance/StarlingMonkey/blob/main/builtins/web/fetch/request-response.cpp#L2021).

Let me know if I can assist further at all with getting the WPT setup working for you.

tschneidereit

This looks really really solid, thank you so much! ❤️

Assuming you get the tests situation sorted out and everything looks good there, I think this is ready to go.

tschneidereit · 2024-11-28T12:28:09Z

builtins/web/fetch/request-response.cpp

+  RootedValue cloned_headers_val(cx, JS::NullValue());
+  RootedObject headers(cx, RequestOrResponse::maybe_headers(self));
+  if (headers) {
+    RootedValue headers_val(cx, ObjectValue(*headers));
+    JSObject *cloned_headers = Headers::create(cx, headers_val, Headers::guard(headers));
+    if (!cloned_headers) {
+      return false;
+    }
+    cloned_headers_val.set(ObjectValue(*cloned_headers));
+  } else if (RequestOrResponse::maybe_handle(self)) {
+    auto handle = RequestOrResponse::headers_handle_clone(cx, self);
+    JSObject *cloned_headers =
+        Headers::create(cx, handle.release(),
+                        RequestOrResponse::is_incoming(self) ? Headers::HeadersGuard::Immutable
+                                                             : Headers::HeadersGuard::Response);
+    if (!cloned_headers) {
+      return false;
+    }
+    cloned_headers_val.set(ObjectValue(*cloned_headers));
+  }


I think you should be able to use RequestOrResponse::headers_handle_clone for this, reducing complexity a bunch. If that's not quite correct to use here, perhaps it can instead be tweaked?

I was unable to work out a simple way to use RequestOrResponse::headers_handle_clone for this, however the solution I've added in a recent commit that constructs a RootedObject based on RequestOrResponse::headers appears to do the trick. I'd love an extra set of eyes on that approach though as I'm still wrapping my head around internal object apis.

RootedObject cloned_headers(cx, RequestOrResponse::headers(cx, self)); if (!cloned_headers) { return false; }

Unfortunately this isn't quite right: it will always return an instance of builtins::web::fetch::Headers tied to self, not a clone of that each time it's called. You should be able to see this if you create a response, clone it using Response.clone, and then do something like response.headers.set("foo", "bar"): this should now show up on both responses.

Can you say what didn't work when using headers_handle_clone?

I gave it another look over and was able to get headers_handle_clone to work for this. I'll chalk it up to my own ignorance regarding the codebase. I've mirrored the changes to Request::clone as well and wrote up some changes to the tests to check that the headers are properly cloned rather than shared.

Once any further changes are worked through, do you prefer keeping the individual commits in this PR or would a force-push of squashed commits be preferred?

tschneidereit · 2024-11-28T12:28:26Z

builtins/web/fetch/request-response.cpp

+  // ENGINE->dump_value(status_val, stderr);
+  // ENGINE->dump_value(status_message_val, stderr);
+


Remove this?

tschneidereit · 2024-11-28T12:28:46Z

builtins/web/fetch/request-response.cpp

+  SetReservedSlot(new_response, static_cast<uint32_t>(Slots::Status), status_val);
+  SetReservedSlot(new_response, static_cast<uint32_t>(Slots::StatusMessage), status_message_val);
+
+    // 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.


Suggested change

// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.

// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.

Accessing can of course be done via the `url` accessor in the base class, this is just here for consistency's sake.

* greatly simplifies the cloning code to rely on `RequestOrResponse::headers`, which appears to do a lot of the necessary work for us. * Also modified `Request.clone` to be consistent with this change. * `Response.clone` now also copies the `URL` slot

zerotri · 2024-12-01T12:55:24Z

The latest commits expose the URL slot to make Response consistent with Request, as well as make sure to copy the value as part of Response.clone. In addition, the code for cloning Response is greatly simplified, so long as constructing a RootedObject from RequestOrResponse::headers actually works as I am hoping it does. Testing the before and after does appear to work as expected.

I was able to get WPT running on my machine and it is reporting:

1: Running test fetch/api/response/response-clone.any.js
1: Sending request to http://127.0.0.1:7676/fetch/api/response/response-clone.any.js
1: wasmtime stderr: stderr [83] :: Warning: Using a DEBUG build. Expect things to be SLOW.
1: wasmtime stdout: stdout [83] :: Log: running test /fetch/api/response/response-clone.any.js
1: fetch/api/response/response-clone.any.js                           6 /   21 (   +0,    -0,    ?0) passing in    0ms

This would imply that some amount of tests are failing, so I will dig into these further to see if I can aim for higher success rate in WPT.

…clone` in order to properly clone headers rather than make a reference to existing headers.

…haring them between clones

Implement Response.clone (bytecodealliance#125)

7b2e0bc

* Implement `Response.clone` This is largely based on the existing `Request.clone` implementation * Add tests for `Response` to match `Request` tests

tschneidereit reviewed Nov 28, 2024

View reviewed changes

zerotri added 2 commits December 1, 2024 03:14

Expose URL slot to Response to match Request.

b4e1371

Accessing can of course be done via the `url` accessor in the base class, this is just here for consistency's sake.

Clean up Response.clone

3ba47dd

* greatly simplifies the cloning code to rely on `RequestOrResponse::headers`, which appears to do a lot of the necessary work for us. * Also modified `Request.clone` to be consistent with this change. * `Response.clone` now also copies the `URL` slot

zerotri added 3 commits December 2, 2024 12:19

Rewriting Response::clone using `RequestOrResponse::headers_handle_…

a783a52

…clone` in order to properly clone headers rather than make a reference to existing headers.

Modifiying Request::clone to match changes to Response::clone

b72a2f0

Modify fetch.js tests to check for headers being cloned rather than s…

f4d1982

…haring them between clones

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Response.clone (#125) #178

Implement Response.clone (#125) #178

zerotri commented Nov 27, 2024

guybedford commented Nov 27, 2024

tschneidereit left a comment

tschneidereit Nov 28, 2024

zerotri Dec 1, 2024

tschneidereit Dec 2, 2024

zerotri Dec 2, 2024

tschneidereit Nov 28, 2024

tschneidereit Nov 28, 2024

zerotri commented Dec 1, 2024

		// ENGINE->dump_value(status_val, stderr);
		// ENGINE->dump_value(status_message_val, stderr);

	// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.
	// 3. If response’s body is non-null, then set newResponse’s body to the result of cloning response’s body.

Implement Response.clone (#125) #178

Are you sure you want to change the base?

Implement Response.clone (#125) #178

Conversation

zerotri commented Nov 27, 2024

guybedford commented Nov 27, 2024

tschneidereit left a comment

Choose a reason for hiding this comment

tschneidereit Nov 28, 2024

Choose a reason for hiding this comment

zerotri Dec 1, 2024

Choose a reason for hiding this comment

tschneidereit Dec 2, 2024

Choose a reason for hiding this comment

zerotri Dec 2, 2024

Choose a reason for hiding this comment

tschneidereit Nov 28, 2024

Choose a reason for hiding this comment

tschneidereit Nov 28, 2024

Choose a reason for hiding this comment

zerotri commented Dec 1, 2024