Skip to content

wasi-http: Fallible fields set and append#7383

Merged
alexcrichton merged 5 commits into
bytecodealliance:mainfrom
elliottt:trevor/fallible-fields-operations
Oct 30, 2023
Merged

wasi-http: Fallible fields set and append#7383
alexcrichton merged 5 commits into
bytecodealliance:mainfrom
elliottt:trevor/fallible-fields-operations

Conversation

@elliottt
Copy link
Copy Markdown
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
Comment thread crates/wasi-http/src/types_impl.rs Outdated
Comment thread crates/wasi-http/src/types.rs Outdated
@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
Comment thread crates/wasi-http/src/types.rs Outdated
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch 2 times, most recently from 23d2a45 to 4bf5e76 Compare October 27, 2023 02:03
Comment thread crates/wasi-http/wit/deps/http/types.wit Outdated
Comment thread crates/wasi-http/wit/deps/http/types.wit Outdated
@elliottt elliottt force-pushed the trevor/fallible-fields-operations branch 6 times, most recently from e9f39e9 to 1df9985 Compare October 27, 2023 18:01
Comment thread crates/wasi-http/wit/deps/http/types.wit Outdated
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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