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

factors-executor: Add ComponentLoader::load_instance_pre #3036

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lann
Copy link
Collaborator

@lann lann commented Mar 4, 2025

This lets embedders customize more of the component loading process.

@lann lann requested a review from rylev March 4, 2025 16:28
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable change for embedders that want to hook into instantiation.

@@ -102,13 +101,23 @@ where

/// A ComponentLoader is responsible for loading Wasmtime [`Component`]s.
#[async_trait]
pub trait ComponentLoader {
pub trait ComponentLoader: Sync {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this now necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The honest answer is "because the compiler says so". I'm pretty sure its some quirk of async_trait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Specifically, up at the load_instance_pre call site I get:

error[E0277]: `impl ComponentLoader` cannot be shared between threads safely
   --> crates/factors-executor/src/lib.rs:71:18
    |
71  |                 .load_instance_pre(&self.core_engine, &component)
    |                  ^^^^^^^^^^^^^^^^^ `impl ComponentLoader` cannot be shared between threads safely
    |
note: required by a bound in `ComponentLoader::load_instance_pre`
   --> crates/factors-executor/src/lib.rs:103:1
    |
103 | #[async_trait]
    | ^^^^^^^^^^^^^^ required by this bound in `ComponentLoader::load_instance_pre`
...
113 |     async fn load_instance_pre<T>(
    |              ----------------- required by a bound in this associated function
    = note: this error originates in the attribute macro `async_trait` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
    |
55  |         component_loader: &impl ComponentLoader + std::marker::Sync,
    |                                                 +++++++++++++++++++

@lann lann force-pushed the load-instance-pre branch 2 times, most recently from 90c7a5d to 897c102 Compare March 4, 2025 17:48
This lets embedders customize more of the component loading process.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@lann lann force-pushed the load-instance-pre branch from 897c102 to b162709 Compare March 4, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants