-
Notifications
You must be signed in to change notification settings - Fork 70
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
NullVm: fix bugs when linking with Rust SDK. #141
Conversation
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
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.
With this change, it looks like we can link Rust SDK with Envoy without modifying Rust SDK itself and run them as NullVms without issues. I think this is awesome!
This is indeed awesome! Thanks for all this work, this is great!
- need to set
Cloneable:CompiledBytecode
for Rust since we need to call_start
on all threads.
You mention Cloneable::CompiledBytecode
, but the only Cloneable
values in this PR are Cloneable::NotCloneable
and Cloneable::InstantiatedModule
.
- need to clean up root contexts created during configuration_canary calls since the code panics due to the duplicated on_create_context call for the same root context id (=1).
What's the difference between plugins written using C++ SDK and Rust SDK here? Do C++ plugins re-use the root context created during canary? That would be wrong.
Basically, I'm trying to understand why this isn't the issue for the existing NullVM C++ plugins.
void kill() { | ||
wasm_base_->startShutdown(); | ||
wasm_base_->finishShutdown(); | ||
wasm_base_ = nullptr; |
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.
Calling finishShutdown()
immediatelly after startShutdown()
, without waiting for the plugin to call proxy_done
is technically incorrect, and effectlively a no-op. This sequence works only if the plugin returns true
from the proxy_on_done
callback, in which case you don't even need to call finishShutdown()
.
It also breaks the promise of not executing anything in the canary (e.g. you can imagine plugins sending batch reports on shutdown, etc.), which is why we're simply dropping the VM right now.
Yeah, actually I was also confused by why C++ works so far. But I notice that C++ SDK DOES allow on_context_create calls for same context ids. In contrast, Rust SDK (and Go SDK as well) panics in that case. This is the why I need to clean up contexts created in the configuration canary phase for Rust SDK. BTW, where do you think is appropriate to put my Rust SDK for NullVm(https://github.com/mathetake/proxy-wasm-rust-nullvm/blob/main/include/proxy-wasm-rust-nullvm/rust_nullvm.h)? Here, or proxy-wasm-rust-sdk repo? NullVm is cpp-host specific stuff but the glue code itself contains Rust specific code, so either seems fine to me. |
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
So basically, including C++ null plugins, on_vm_start and on_configure are called twice ( canary + the usual startup time call). I think we should fix this not only for Rust but also C++. |
this should be fixed when we removing the need for canary. Closing. |
I have been playing around with Rust SDK in my spare time and statically linking plugins in Rust with Envoy. I found that some changes must be made in the cpp-host to resolve the Rust SDK specific issues:
need to setfixed in the Rust Null VM SDK(https://github.com/mathetake/proxy-wasm-rust-nullvm/commit/ddfe09fb8914f93acc2439f00a6e486cc90856bc)Cloneable:CompiledBytecode
for Rust since we need to call_start
on all threads.This is still WIP since I haven't figured out how to fix Envoy tests, but all of them are uninteresting mock function calls and can be easily fixed.
With this change, it looks like we can link Rust SDK with Envoy without modifying Rust SDK itself and run them as NullVms without issues. I think this is awesome!
FYI, we can link multiple plugins at the same time without symbol collision with my simple trick here 🙂
ref:
Signed-off-by: Takeshi Yoneda takeshi@tetrate.io