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-sockets: Add network-error-code #9276

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

badeend
Copy link
Contributor

@badeend badeend commented Sep 18, 2024

Implements WebAssembly/wasi-sockets#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.

@badeend badeend requested a review from a team as a code owner September 18, 2024 09:32
@badeend badeend requested review from elliottt and removed request for a team September 18, 2024 09:32
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Sep 18, 2024
@badeend badeend requested a review from a team as a code owner September 18, 2024 12:14
@alexcrichton
Copy link
Member

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?

@badeend
Copy link
Contributor Author

badeend commented Sep 18, 2024

I'll give it a try

@badeend
Copy link
Contributor Author

badeend commented Sep 18, 2024

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.

@alexcrichton
Copy link
Member

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)

@elliottt elliottt requested review from alexcrichton and removed request for elliottt September 25, 2024 20:20
@badeend
Copy link
Contributor Author

badeend commented Oct 1, 2024

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:

"wasi:clocks": crate::bindings::clocks,
"wasi:random": crate::bindings::random,
"wasi:cli": crate::bindings::cli,
"wasi:io/error": crate::bindings::io::error,
"wasi:filesystem/preopens": crate::bindings::filesystem::preopens,
"wasi:sockets/network": crate::bindings::sockets::network,

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?

@alexcrichton
Copy link
Member

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.

@badeend
Copy link
Contributor Author

badeend commented Oct 1, 2024

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

@alexcrichton
Copy link
Member

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

@badeend
Copy link
Contributor Author

badeend commented Oct 1, 2024

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.

@alexcrichton
Copy link
Member

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.

@badeend
Copy link
Contributor Author

badeend commented Oct 6, 2024

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: #9381

…imilar to the existing filesystem-error-code and http-error-code functions.
@badeend
Copy link
Contributor Author

badeend commented Oct 9, 2024

I've updated the PR with the latest main

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexcrichton alexcrichton added this pull request to the merge queue Oct 9, 2024
Merged via the queue into bytecodealliance:main with commit cef7774 Oct 9, 2024
39 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.

2 participants