Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update from upstream - Fetch reworks #5

Merged
merged 30 commits into from
Aug 1, 2024
Merged

Update from upstream - Fetch reworks #5

merged 30 commits into from
Aug 1, 2024

Conversation

noise64
Copy link

@noise64 noise64 commented Aug 1, 2024

guybedford and others added 30 commits June 5, 2024 14:13
This is a large-scale rewrite of significant parts of the fetch API implementation that addresses a range of shortcomings and bugs in the previous code.

Key changes are a better abstraction of host handles in the `host_api.h` header, cleaner handling of all the different object types involved in the fetch API, improved handling of strings and streams, and a whole bunch of bug fixes all over the place.

Additionally, this includes various build system tweaks and some facilities for making debugging easier, such as the introduction of a way to build a component that doesn't include pre-evaluated JS and instead supports runtime evaluation of a script supplied in the incoming request's body.
This also applies some cleanups to Response creation overall.
Specifically, some of the `noGC` guards had issues that could lead to panics during exception throwing, and some of the loops over multiple buffers where overly complex.
Before this patch, timer (i.e., `setTimeout`/`setInterval`) returned the pollable handles as IDs. This is problematic, because pollable handles are reused, leading to content potentially clearing the wrong timer.

Additionally, the code wasn't robust against a timer being cleared in its own callback, leading to a failing assertion.

-----

This PR also includes the following changes:

* Introduce an explicitly async test variant to the integration tests framework

This cuts down on redundancy and is a bit easier to use, IMO.

I didn't change all tests to use this instead of the existing async support, but we could consider doing that at some point.

* Fix issues with timer subtest

The test didn't properly clean up its timeout and interval, which meant that the former would throw an exception into the ether, and the latter would continue running the interval forever.

(The exception thrown by the former also happened to be the wrong one, since `AssertionError` wasn't imported.)
This introduces support for adding typed error definitions in a modular way, such that extensions can define their own typed errors.

This support is then also used in a few places, though more work can be done there.
…e#73)

* Change all error reporting to use `api::throw_error`

I went through the existing errors with a pretty fine comb and cleaned things up where it makes sense, instead of just moving things over mechanically. As a result, a few more WPT tests pass, and in general things are a bunch cleaner now, I think.

* Address review comments

* Fix test expectations
* Update to SpiderMonkey v127.0.2

This commit comes from bytecodealliance/spidermonkey-wasi-embedding#13

* Update main.yml

* always use link-time-optimisations for rust-url dependency
This fixes a crashing bug in a trivial scenario: event.respondWith(new Response(someUpstreamResponse.body, someUpstreamResponse);

It's not entirely clear to me why WPT didn't catch this, though it's possible that the tests that would've done so abort early for trivial reasons, such as our missing FormData support.

Instead, I added an e2e test to cover this, which fails without this patch.

(There are small unrelated tweaks to the runtime-eval support which I applied in debugging the test.)
…etch-rework

# Conflicts:
#	README.md
#	builtins/web/fetch/fetch-api.cpp
#	builtins/web/fetch/fetch_event.cpp
#	builtins/web/fetch/headers.cpp
#	builtins/web/fetch/headers.h
#	builtins/web/fetch/request-response.cpp
#	builtins/web/fetch/request-response.h
#	host-apis/wasi-0.2.0-rc-2023-10-18/host_api.cpp
#	host-apis/wasi-0.2.0-rc-2023-12-05/host_api.cpp
#	host-apis/wasi-0.2.0/host_api.cpp
#	include/extension-api.h
#	runtime/event_loop.cpp
#	tests/tests.cmake
#	tests/wpt-harness/wpt.cmake
# Conflicts:
#	builtins/web/fetch/request-response.cpp
@noise64 noise64 merged commit 97ba944 into main Aug 1, 2024
2 checks passed
@noise64 noise64 deleted the fetch-rework branch August 1, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants