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

wasi-http: Fallible fields set and append #7383

Merged

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Oct 26, 2023

This PR makes the fields.set and fields.append methods fallible, and introduces a new error type for describing the ways in which those operations can fail. Additionally it adds a notion of forbidden headers, which will always include the following:

  • Connection
  • Keep-Alive
  • Proxy-Authenticate
  • Proxy-Authorization
  • Proxy-Connection
  • TE
  • Transfer-Encoding
  • Upgrade
  • HTTP2-Settings

There's an embedder-defined hook for adding additional header name validation, but that underlying set of headers can't be altered.

Fixes #7270

@elliottt elliottt requested a review from a team as a code owner October 26, 2023 18:06
@elliottt elliottt requested review from fitzgen and pchickey and removed request for a team October 26, 2023 18:06
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Oct 26, 2023
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch from 36e1f95 to fe05c4d Compare October 26, 2023 22:59
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch 2 times, most recently from 23d2a45 to 4bf5e76 Compare October 27, 2023 02:03
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch 6 times, most recently from e9f39e9 to 1df9985 Compare October 27, 2023 18:01
Copy link
Collaborator

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Looks great overall - rather than trap in the fields constructor, lets make the constructor create an empty set of fields, and have a static function that takes the list<tuple<...>> repr and returns result<fields, header-error>

@lukewagner
Copy link
Collaborator

@pchickey I think (but may be missing a case), that, with the eager-validation changes Trevor made most recently, there isn't a header-related error that can occur in the constructor?

Independently, the discussion made me think that perhaps (in a post-Preview-2 timeframe) we should relax the C-M constructor validation rules to allow constructor functions to return a result<containing-resource-type, T> (for any T). In languages with EH, this would map to the usual throwing-constructor pattern. Otherwise, bindings would produce their best approximation of a fallible constructor.

@pchickey
Copy link
Collaborator

pchickey commented Oct 30, 2023

The function set: func(name: field-key, value: list<field-value>) -> result<_, header-error>; allows rejecting input, but constructor(entries: list<tuple<field-key,field-value>>); would not allow that same input to be rejected with an error, so currently the implementation traps.

I agree we should allow constructors to throw in a post-p2 timeframe.

@lukewagner
Copy link
Collaborator

Ah, gotcha; I was thinking about the case of passing in the already-constructed headers.

@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch from 8ce8c72 to 4269e91 Compare October 30, 2023 18:08
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch from 4269e91 to 66ad714 Compare October 30, 2023 18:56
@elliottt elliottt added this pull request to the merge queue Oct 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Oct 30, 2023
Merged via the queue into bytecodealliance:main with commit c97f5d6 Oct 30, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasi-http: make header methods fallible
4 participants