dispatchrun / wasi-go

A Go implementation of the WebAssembly System Interface (WASI)
Apache License 2.0
124 stars 7 forks source link

Add lock to protect concurrency #93

Closed Taction closed 1 year ago

Taction commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/stealthrocket/wasi-go/issues/87#issuecomment-1644768227

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 commented 1 year ago

~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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Thanks!