-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
Signed-off-by: zhangchao <[email protected]>
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. |
I looked into this somewhat, and here are the details. In the Dapr codebase, And then the module is instantiated in I realized that I'm not perfectly clear about this comment: Does it imply that a If not, the right fix is in the Dapr codebase. |
I take it back this is documented in wazero here: https://github.com/tetratelabs/wazero/blob/main/examples/concurrent-instantiation/main.go |
Ok, after a little more research, I believe that this fix is correct for concurrent execution of multiple instantiated modules in the same Thanks! |
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. |
@achille-roussel I think this is fine to merge whenever you get a chance. Thanks! |
There was a problem hiding this 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!
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