Add WCGI to wasmer run #3613
Add WCGI to wasmer run #3613
Conversation
| /// file's default entrypoint. | ||
| /// | ||
| /// This will infer the [`CgiDialect`] from the WEBC file's metadata | ||
| pub fn build_webc(self, webc: impl Into<Bytes>) -> Result<Runner, Error> { |
There was a problem hiding this comment.
I don't think build_webc is a good name. Probably use_webc or from_webc are way better names
There was a problem hiding this comment.
Also, you may want to provide a command to execute, rather than infer it. And get the webc directly instead of the bytes.
That way there are two steps:
- parsing the webc
- detecting the command to run and what kind it is
- Run the runner
There was a problem hiding this comment.
I deliberately named it that way so we can follow the Runner::builder().foo().bar().build() pattern. Using "use" or "from" would be very confusing.
In this case, we have multiple ways of constructing the final runner (e.g. a WEBC file, a WebAssembly binary, or the filename for something on disk), so there are multiple build_xxx() methods.
There was a problem hiding this comment.
I see the point that the build_webc actually returns a Runner (which I didn't realize before), so I kind of agree on that naming if we want to return a runner.
However, I think the webc shall be parsed beforehand and then chose the command to run.
For example, let's say you have a package with two commands:
- A (WASI command)
- B (WCGI command)
You don't know which runner you are going to run, until you have parsed the webc package. And at that point you already have the config and such. So passing a stream of bytes to parse again the webc seems wrong
| let rt = wasmer_wasi::runtime::task_manager::tokio::TokioTaskManager::default(); | ||
|
|
||
| let mut builder = Runner::builder() | ||
| .map_dirs(mapped_dirs) |
There was a problem hiding this comment.
I'd put here: with_wasi_config(CONFIG). Because that way it can be reused in the future with the WASI runner
| use crate::{ | ||
| module_loader::{ModuleLoader, ModuleLoaderContext}, | ||
| Error, | ||
| }; | ||
|
|
||
| use super::LoadedModule; | ||
|
|
||
| pub(crate) struct Cached<L> { | ||
| loader: L, | ||
| invalidated: Box<dyn Fn() -> bool + Send + Sync>, | ||
| cached: Mutex<Option<LoadedModule>>, | ||
| } | ||
|
|
||
| impl<L> Cached<L> { | ||
| pub(crate) fn new(loader: L, invalidated: impl Fn() -> bool + Send + Sync + 'static) -> Self { | ||
| Self { | ||
| loader, | ||
| invalidated: Box::new(invalidated), | ||
| cached: Mutex::new(None), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl<L: ModuleLoader> ModuleLoader for Cached<L> { | ||
| async fn load(&self, ctx: ModuleLoaderContext<'_>) -> Result<LoadedModule, Error> { | ||
| let mut cached = self.cached.lock().await; | ||
|
|
There was a problem hiding this comment.
Why do we need this exactly? We already have the wasmer-cache crate that should handle this
There was a problem hiding this comment.
Regardless of whether we use a Mutex<Option<LoadedModule>> or a wasmer_cache::FileSystemCache, we still need to have some sort of struct which can be used as a ModuleLoader.
I used a mutex-based solution just because it's trivial to write and was good enough to get things implemented.
There was a problem hiding this comment.
But the wasmer_cache::FileSystemCache is already provided by the CLI, right?
I don't think the caching mechanism should be defined in the runner (but plugged into it).
I think we might be doing an early optimization by having the wcgi-runner standalone instead of being first integrated on the cli first (which I think might lead to simpler implementation)
|
After a bit of discussion today on Slack, I'm going to close this and move all changes to #3599. We need to redesign how the WCGI runner works because the current design doesn't match how we want |
This adds the WCGI runner introduced in #3599 to the
wasmerCLI.