wasmtime / PR #9276 wasi-sockets: Add `network-error-code` · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9276 wasi-sockets: Add `network-error-code`

Wasmtime GitHub notifications bot (Sep 18 2024 at 09:32):

badeend requested elliottt for a review on PR #9276.

Wasmtime GitHub notifications bot (Sep 18 2024 at 09:32):

badeend requested wasmtime-core-reviewers for a review on PR #9276.

Wasmtime GitHub notifications bot (Sep 18 2024 at 09:32):

badeend opened PR #9276 from badeend:network-error-code to bytecodealliance:main:

Implements https://github.com/WebAssembly/wasi-sockets/pull/105

This function downcasts a stream error to a network-related error.
Similar to the existing filesystem-error-code and http-error-code functions.

Unit test is missing because I couldn't figure out a reliable way to fail a TCP stream mid-connection. Suggestions are welcome.

Wasmtime GitHub notifications bot (Sep 18 2024 at 12:14):

badeend requested wasmtime-default-reviewers for a review on PR #9276.

Wasmtime GitHub notifications bot (Sep 18 2024 at 12:14):

badeend updated PR #9276.

Wasmtime GitHub notifications bot (Sep 18 2024 at 15:16):

alexcrichton commented on PR #9276:

Thanks for this! Before I think too hard about a unit test though this PR is the unfortunate victim of "first proposal to add WITs with @unstable". As implemented this provides unrestricted access to the @unstable version of the API which is not the intention of the annotation. The theory is that @unstable would translate to some sort of -S flag such as -Snetwork-error-code or something like that. That would be there during development and then when @unstable is turned to stable then -Snetwork-error-code becomes a noop effectively.

I say that this is an "unfortunate victim" because this infrastructure hasn't been built yet. Ideally what needs to happen I think is that the calls to add_to_linker all need to be parameterized by feature flags where the default is the empty set. So in a sense we need a set of flags for all WASI proposals which are passed during the original call to add_to_linker and then that set is built up on the CLI.

From a technical perspective though this PR otherwise looks good to me. Would you be up for helping out with the infrastructure for plumbing flags through?

Wasmtime GitHub notifications bot (Sep 18 2024 at 15:43):

badeend commented on PR #9276:

I'll give it a try

Wasmtime GitHub notifications bot (Sep 18 2024 at 18:53):

badeend commented on PR #9276:

Do you have any preference on how to thread through the feature names down into the generated code? I have three suggestions so far:

Add component_model_unstable_features setting to wasmtime::Config

Add unstable_features setting to wasmtime::component::Linker

Add features parameter to generated add_to_linker_* methods and all layers below and above it.

Wasmtime GitHub notifications bot (Sep 18 2024 at 19:02):

alexcrichton commented on PR #9276:

I'd recommend the add_to_linker_* route and it would automatically be used when features is specified in the bindgen! macro (it's a WASI-specific or bindings-specific thing, less about engine state)

Wasmtime GitHub notifications bot (Sep 25 2024 at 20:20):

elliottt requested alexcrichton for a review on PR #9276.

Wasmtime GitHub notifications bot (Oct 01 2024 at 19:24):

badeend commented on PR #9276:

I'm attempting to only generate the new parameter on add_to_linker _if_ the bindgen config specifies unstable features. I'm running into a problem when the with option is used to forward to existing interfaces. For example:

https://github.com/bytecodealliance/wasmtime/blob/9fc41baefb70c87e767a1cd2a22b961629ecb576/crates/wasi/src/bindings.rs#L161-L166

Within the bindgen macro I don't know for which of those interfaces I need to forward the features parameter to their respective add_to_linker call.

One way to make it work is to simply unconditionally add the parameter to every generated add_to_linker function, regardless of wheter its needed or not. But that will be a backwards incompatible change.

Suggestions?

Wasmtime GitHub notifications bot (Oct 01 2024 at 19:49):

alexcrichton commented on PR #9276:

IIRC there are per-interface add_to_linker functions plus one large "add everything to the linker" function. I'd expect that each per-interface function has a flags/options struct as appropriate for the contents of the interface, and then the one large function would take the union of all the options of all the other interfaces. Would that be reasonable to implement?

Even when with is used we could say that the detection of whether or not the flags are used is independent of with configuration and if the flags type itself lived in the module generated it would be able to reuse the type that with points to I think.

Wasmtime GitHub notifications bot (Oct 01 2024 at 20:04):

badeend commented on PR #9276:

If every interface defines its own fresh options type, then a bindgen! using with must generate a separate parameter for each "with"ed interface, right?

So the example from above would produce
add_to_linker(
  linker: Linker,
  clocks_options: crate::bindings::clocks::Options,
  random_options: crate::bindings::random::Options,
  cli_options: crate::bindings::cli::Options,
  etc...
)

Wasmtime GitHub notifications bot (Oct 01 2024 at 20:15):

alexcrichton commented on PR #9276:

I might bikeshed to a struct WorldOptions { /* all the fields here */ } to avoid needing to remember what order everything is in, but otherwise yeah the top-level function would take all the flags for sure

Wasmtime GitHub notifications bot (Oct 01 2024 at 20:19):

badeend commented on PR #9276:

For the world that could work. But I'm specifically concerned here about the with-included interfaces, which come from a different code generation and don't share the options type of the currently generating world type.

Wasmtime GitHub notifications bot (Oct 01 2024 at 20:56):

alexcrichton commented on PR #9276:

Could you detail a bit more how with complicates this? From my perspective there's no extra complications, so I'm not sure what you're meaning. The bindgen! using with still has full WIT type information so should be able to know, even without knowing what was generated, that some of the interfaces on the other end are unstable-gated. That means that it should know that there's an Options type already generated and the with says how to go find that type. Given that and the fact that bindgen! would always generate a fresh the-whole-world's-options-type it seems naively to me like it would work, but I fear I'm missing something.

One possible piece perhaps is that I'm thinking now that Wasmtime might want to unconditionally always use all_features and remove that macro option entirely. It would then always generate the Options structs for unstable-gated interfaces no matter what, so features/all_features in the macro config would go away.

Wasmtime GitHub notifications bot (Oct 06 2024 at 10:43):

badeend commented on PR #9276:

The bindgen! using with still has full WIT type information so should be able to know, even without knowing what was generated, that some of the interfaces on the other end are unstable-gated.

You're right. I've got it working in: https://github.com/bytecodealliance/wasmtime/pull/9381

Wasmtime GitHub notifications bot (Oct 09 2024 at 15:14):

badeend updated PR #9276.

Wasmtime GitHub notifications bot (Oct 09 2024 at 15:22):

badeend commented on PR #9276:

I've updated the PR with the latest main

Wasmtime GitHub notifications bot (Oct 09 2024 at 16:00):

alexcrichton submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Oct 09 2024 at 16:23):

alexcrichton merged PR #9276.

Last updated: Oct 23 2024 at 20:03 UTC