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

Remove Send bound from FunctionEnv #4634

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

Conversation

Twey
Copy link
Contributor

@Twey Twey commented May 1, 2024

Description

Requiring that function environments be Send is quite onerous on the Web, in which many objects (including the Wasmer Instance itself, should we wish to pass around a reference to it) are not Send. The function environment is not passed between threads in Wasmer, and in typed usage the user never sees the trait object, only concrete instances of it after casting — which means that all user-visible types will remain Send if the user should wish to pass them around.

@Twey Twey requested a review from syrusakbary as a code owner May 1, 2024 15:31
@Twey Twey force-pushed the non-send-environments branch 3 times, most recently from 66b136e to 2e48e6b Compare May 1, 2024 16:21
@Twey
Copy link
Contributor Author

Twey commented May 17, 2024

Hi @syrusakbary, the system automatically assigned you to review this — but if somebody else should do it please let me know whom to request 😃

@Twey Twey force-pushed the non-send-environments branch from 2e48e6b to 7cb4178 Compare May 17, 2024 14:59
@syrusakbary
Copy link
Member

Hey @Twey , we want to set a sound type system for the Store, in a way that if the Store is Send+Sync then the functions have to be as well. But if is not... then they don't need to.

But we need to define a bit better this, that's why the review on this PR is a bit halted

@Twey
Copy link
Contributor Author

Twey commented Jun 5, 2024

Hey @syrusakbary, thanks for getting back to me! 😄

The easiest way I can think of to do that would be to remove the Box<dyn Any> from the VMFunctionEnvironment struct and make it a parameter instead (which could be instantiated to Box<dyn Any> or Box<dyn Any + Send> as the user likes — or kept as the concrete T that will be used at a higher level). That way both the store and the function environment will inherit their Sendness from the parameter. Do you think that's worth me making a PR for? Or is there a reason we can't have type parameters on VMFunctionEnvironment?

@Twey Twey force-pushed the non-send-environments branch from 7cb4178 to 1906b16 Compare June 5, 2024 20:04
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.

3 participants