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

Is it intentional that bi-directional communication is partially supported? #24

Open
cuongvo opened this issue Apr 20, 2022 · 6 comments

Comments

@cuongvo
Copy link

cuongvo commented Apr 20, 2022

Per the waPC specifications it looks like after the guest is invoked initially, the supported flow is the guest is allowed to make 0..N __host_call before returning.

There's another flow, where communication is bi-directional, or where multiple __guest_calls occur from a single invoke:

  • Guest is invoked
  • Guest does some work, and calls host
  • Host starts a new __guest_call
  • ... new flow starts. Guest eventually returns results to host
  • Host returns results to guest
  • Guest returns

This flow currently fails for the final __guest_call in wasmer (assumption is it also fails in wasmtime), but works for wazero. This is due to the use of invokeContext. In wasmer / wasmtime this is a pointer stored on the shared instance and the response copied to it (here), but the return value of invoke is read from the original invokeContext (here). With multiple __guest_call in a single flow, the shared context is replaced. The output is copied to the invokeContext of the previous __guest_call and the final __guest_call in the chain returns a zero value.

wazero supports this flow since each __guest_call via invoke using wazero gets its own context, not a shared context on the instance (here).

Whew, with that out of the way. Is the above bi-directional flow supported? If not, should it be supported and the implementation for wasmer and wasmtime changed to allow it?

@pkedy
Copy link
Member

pkedy commented Apr 22, 2022

Whew, with that out of the way. Is the above bi-directional flow supported? If not, should it be supported and the implementation for wasmer and wasmtime changed to allow it?

Thanks for reporting this and the detailed explanation! This bi-directional communication with several back and forth hops absolutely should be supported. I'll take a look at this.

@codefromthecrypt
Copy link
Contributor

warning, these are just drive-by thoughts from someone only using go for <1.5years, and also I work on wazero.

I'm not sure any CGO lib would be able to propagate go context.. if anything some pointer magic and the lib would be very aware of it. I've not seen anything in the rust projects (wasmer, wasmtime) that would do it.

An alternative could be to change wapc's callback apis to add a u64 param and require it as a sort of correlation context parameter. you could for example store an unsafe pointer of a context.Context between calls that way, and look it up.

A less api effecting way could be to use something like go-eden for context-storage. When CGO is already in use, it claims to have an efficient mechanism that internally correlates data based on a goroutine ID. If you use that, be careful to not make it a required dep for all wasm runtimes and also watch for supported arch/os because the non-CGO alternative is documented (and looks) expensive https://github.com/go-eden/routine/blob/b65d4d238e09c18ba6470bb5731f514b8068392b/routine_goid.go#L63

hope this helps!

@codefromthecrypt
Copy link
Contributor

@cuongvo would you be ok with contributing (even pasting here) a failing unit test for the flow? One that would go into the main test file?

@cuongvo
Copy link
Author

cuongvo commented Jun 1, 2022

Apologies, just saw this. Sure, I can provide a failing unit test to demonstrate this.

@cuongvo
Copy link
Author

cuongvo commented Jun 2, 2022

@codefromthecrypt Here's a commit that shows the error: d60104a

I removed wasmtime from the engines since I didn't have time to look into getting it running on darwin / arm64. wasmer fails though as expected while wazero passes.

=== RUN   TestBidirectionalEngineCalls
--- FAIL: TestBidirectionalEngineCalls (0.02s)
=== RUN   TestBidirectionalEngineCalls/wasmer
    --- FAIL: TestBidirectionalEngineCalls/wasmer (0.00s)
=== RUN   TestBidirectionalEngineCalls/wasmer/Bidirectional_testing_with_assemblyscript_Guest
        --- FAIL: TestBidirectionalEngineCalls/wasmer/Bidirectional_testing_with_assemblyscript_Guest (0.00s)
=== RUN   TestBidirectionalEngineCalls/wasmer/Bidirectional_testing_with_assemblyscript_Guest/Calls_Function_Bi-Directionally
    engine_test.go:195: Unexpected response message, got , expected aaaaaaaa
            --- FAIL: TestBidirectionalEngineCalls/wasmer/Bidirectional_testing_with_assemblyscript_Guest/Calls_Function_Bi-Directionally (0.00s)



=== RUN   TestBidirectionalEngineCalls/wazero
    --- PASS: TestBidirectionalEngineCalls/wazero (0.01s)
=== RUN   TestBidirectionalEngineCalls/wazero/Bidirectional_testing_with_assemblyscript_Guest
        --- PASS: TestBidirectionalEngineCalls/wazero/Bidirectional_testing_with_assemblyscript_Guest (0.01s)
=== RUN   TestBidirectionalEngineCalls/wazero/Bidirectional_testing_with_assemblyscript_Guest/Calls_Function_Bi-Directionally
            --- PASS: TestBidirectionalEngineCalls/wazero/Bidirectional_testing_with_assemblyscript_Guest/Calls_Function_Bi-Directionally (0.00s)

@atifsyedali
Copy link

I hit a similar problem and found that wastime works but wazero and wasmer do not. Additionally, the built-in wasm runtime in browsers also do not exhibit the same problem. I did not run the tests above on wasmtime.

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

No branches or pull requests

4 participants