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

Add lock to protect concurrency #93

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

Taction
Copy link
Contributor

@Taction Taction commented Aug 14, 2023

When I test wasi-http in dapr, I found there was still concurrent map writes errors: https://github.com/dapr/components-contrib/actions/runs/5851405294/job/15862279360?pr=3077. I have tried using the latest sdk version in my local enviroment, but this issue still appears.

So I added lock to protect the map since there could be multi wasm instances, this works fine in my local environment.

also related to #87

@brendandburns
Copy link
Contributor

This code looks good to me, but I actually think I may have written the test wrong...

I will look at the dapr test and see if there's a way to fix it there also.

@brendandburns
Copy link
Contributor

I looked into this somewhat, and here are the details.

In the Dapr codebase, NewRuntimeWithConfig is called in the Init function:
https://github.com/dapr/components-contrib/blob/master/bindings/wasm/output.go#L73
The module is also compiled in Init

And then the module is instantiated in Invoke which I believe can run in parallel:
https://github.com/dapr/components-contrib/blob/master/bindings/wasm/output.go#L136

I realized that I'm not perfectly clear about this comment:
#87 (comment)

Does it imply that a Runtime can be used concurrently for multiple InstantiateModule calls? If so, this PR is the right fix.

If not, the right fix is in the Dapr codebase.

@brendandburns
Copy link
Contributor

brendandburns commented Aug 14, 2023

Given that a Runtime has an associated Store (https://github.com/tetratelabs/wazero/blob/main/RATIONALE.md#runtime--enginestore) I think that implies that a Runtime should not be multi-threaded, but I'm still not positive.

I take it back this is documented in wazero here: https://github.com/tetratelabs/wazero/blob/main/examples/concurrent-instantiation/main.go

@brendandburns
Copy link
Contributor

Ok, after a little more research, I believe that this fix is correct for concurrent execution of multiple instantiated modules in the same Runtime

Thanks!

@Taction
Copy link
Contributor Author

Taction commented Aug 15, 2023

Thanks for your patience and the research. In my humble opinion, using multi runtime instances would consume more (memory mainly) resources than instantiated multi wasm instances. And using one wasm instance which means handling requests one by one is not acceptable in the production environment.

@brendandburns
Copy link
Contributor

@achille-roussel I think this is fine to merge whenever you get a chance.

Thanks!

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@achille-roussel achille-roussel merged commit 8efbab5 into dispatchrun:main Sep 12, 2023
3 checks passed
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