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

fix: headers refactoring and state transition invariants #80

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jul 22, 2024

This includes some simple refactoring including unused code paths and also adds some extra state assertions, including encoding the state transition invariant that we never transition from Uninitialized to HostOnly. If this invariant changes in future we can always relax it again, but it helped my understanding of the state machine to note these assertions.

@guybedford guybedford changed the title fix: unused code and state transition invariants fix: headers unused code and state transition invariants Jul 22, 2024
@guybedford guybedford changed the title fix: headers unused code and state transition invariants fix: headers refactoring and state transition invariants Jul 22, 2024
@@ -111,7 +111,7 @@ class Headers final : public BuiltinImpl<Headers> {
static JSObject *create(JSContext *cx, host_api::HttpHeadersReadOnly *handle,
host_api::HttpHeadersGuard guard);

static JSObject *init_entries(JSContext *cx, HandleObject self, HandleValue init_headers);
static bool init_entries(JSContext *cx, HandleObject self, HandleValue init_headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make a decision at some point about how we version starlingmonkey and how we communicate public function signatures changes such as this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From js-compute-runtime's perspective, every StarlingMonkey commit is potentially a breaking change. There simply are no API guarantees.

But since these are changes on the js-compute-runtime -> StarlingMonkey interface, they are all manageable in theory.

Problems come into play if:

  1. A StarlingMonkey change causes the corresponding js-compute-runtime upgrade to be an inhibitative amount of work.
  2. A StarlingMonkey change causes a breaking change in js-compute-runtimes's public guest API usage for JS users.

I hope our implicit agreement is effectively that we could revert a change in the case of (1) or (2), but perhaps we should discuss this more with @tschneidereit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this as a sentiment for now, yes. Eventually I would like to get to a more stable API, but I don't think the one we have right now is sufficiently well designed to declare it stable as-is.

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good insight, thank you!

I think there's a slight change still to be made—see the comments in-line, but then it'll be good to go.

builtins/web/fetch/headers.cpp Outdated Show resolved Hide resolved
builtins/web/fetch/headers.cpp Show resolved Hide resolved
@guybedford guybedford merged commit 102ffbe into main Jul 24, 2024
1 check passed
@guybedford guybedford deleted the invariant-tweaks branch July 24, 2024 18:26
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.

3 participants