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

wasmer-wasix@4 does not allow to instantiate without http client #4029

Closed
kwonoj opened this issue Jun 23, 2023 · 4 comments · Fixed by #4055
Closed

wasmer-wasix@4 does not allow to instantiate without http client #4029

kwonoj opened this issue Jun 23, 2023 · 4 comments · Fixed by #4055
Assignees
Labels
🎉 enhancement New feature! priority-medium Medium priority issue
Milestone

Comments

@kwonoj
Copy link
Contributor

kwonoj commented Jun 23, 2023

Thanks for proposing a new feature!

Motivation

We have limited wasix support in the runtime, by enabling some of wasix features like

[
"wasmer-wasix/sys",
  "wasmer-wasix/logging",
  "wasmer-wasix/host-fs",
  "wasmer-wasix/sys-poll",
  "wasmer-wasix/sys-thread",
  "wasmer-wasix/host-threads",
]

With wasmer@4, this no longer works since default_http_client does not return instance if host-reqwest is not enabled. Instead, it raises build time error

panicked at 'Loading the builtin resolver should never fail: No HTTP client available'

I would agree having a default network client is a reasonable default, but in certain cases like us bringing whole reqwest impl into binary is not something easily able to adapt. Would like to have a way to instantiate runtime without it.

Proposed solution

Make a no-op client if backing http client is not enabled; or in a high level networking itself could be opt-in.

Or, provide an interface to provide http client dynamically when init runtime, so the caller can provide no-op instance if they want.

Alternatives

Unsure if there's a workaround without upstream changes.

Additional context

Primary reason we want to avoid reqwest (and potentially others) is it is not trivial to have enabled for the all of the target platform we support, and also we have quite complex cargo feature conflict when bring those into.

@kwonoj kwonoj added the 🎉 enhancement New feature! label Jun 23, 2023
@theduke
Copy link
Contributor

theduke commented Jun 23, 2023

So the background here is that the runtime HTTP client is also used for fetching packages from the registry, to make things work in the browser.

It should not be needed for just executing wasm files, and the Runtime::http_client() method already returns Option<_> to reflect that fact.

It's definitely possible that we try to force-use an http client in a situation where it isn't strictly neccessary.

In what situation are you running into issues?
Can you give a code example?

ps: networking is already opt in, there is a UnsupportedVirtualNetworking struct that implements VirtualNetworking but fails every network operation.

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 23, 2023

This is WIP branch shows the error: https://github.com/kwonoj/swc/tree/feat-wasmer-4 with cargo test -p swc_plugin_runner --features plugin_transform_schema_v1 --features rkyv-impl --features ecma --features css

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 23, 2023

So far I read, it seems it's coming from

crate::http::default_http_client().map(|client| Arc::new(client) as DynHttpClient);
let loader = BuiltinPackageLoader::from_env()
.expect("Loading the builtin resolver should never fail");
when pluggableruntime is being created and try to read default_http_client.

@theduke
Copy link
Contributor

theduke commented Jun 23, 2023

Ah yeah, well, since you won't need that package loader (which is only for downloading packages from a package registry), you can either just manually construct the PluggableRuntime (all fields are public), or implement Runtime for you own custom struct.

And you can use a noop trait PackageLoader implementation.

We should make the PackageLoader optional though, just like the http client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! priority-medium Medium priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants