invoke on_context_create_ for all root contexts with a same VM ID #64

mathetake · 2020-10-05T07:54:41Z

Fix the bug in using a samewasm file for multiple times with a same VM ID:

on_context_create_ is invoked only for the first root context (this is because on_context_create_ is executed in onStart)
onConfigure is executed for the other root contexts without invoking on_context_create_,

As far as I know, this results in panics in Rust and TinyGo SDKs, and is not the intended behavior.

Signed-off-by: mathetake takeshi@tetrate.io

lizan

Let's add some test too?

@PiotrSikora PTAL.

PiotrSikora · 2020-10-09T22:08:24Z

Fix the bug in the shared vm situation:

on_context_create_ is invoked only for the first root context (this is because on_context_create_ is executed in onStart)

onConfigure is executed for the other root contexts without invoking on_context_create_,

As far as I know, this results in panics in Rust and TinyGo SDKs, and is not the intended behavior.

I think this is fine, but what exactly do you mean by the "Shared VM" situation? Are you loading different .wasm files into the same WasmVm? Or the same .wasm file with multiple plugins within?

mathetake · 2020-10-11T23:38:56Z

I mean the same .wasm file with the multiple plugin within or the same .wasm behaving differently corresponding to the plugin configurations. AFAIK, using different wasm files with same VM ID introduces unexpected behavior and is not supported. Is that correct? Anyway, I changed the PR's title and description for clarity.Thanks!

PiotrSikora · 2020-10-13T20:42:20Z

Let's add some test too?

In interest of unblocking this, are we waiting for any tests in this PR?

PiotrSikora · 2020-10-13T20:43:56Z

I mean the same .wasm file with the multiple plugin within or the same .wasm behaving differently corresponding to the plugin configurations. AFAIK, using different wasm files with same VM ID introduces unexpected behavior and is not supported. Is that correct?

Correct, it's not supported, since then different allocators wouldn't know about allocations done by other plugins and would overwrite each other's memory. I just wanted to make sure that's not something you wanted to try to support with "shared VMs". Thanks for claryfing.

mathetake · 2020-10-13T23:57:17Z

In interest of unblocking this, are we waiting for any tests in this PR?

Sorry @PiotrSikora, I have no plan to implement the test in this PR since it requires a lot of work as I discussed with @lizan privately. Is that ok for you?

But I desperately want to have test infrastructure in this repository like the comment here (#47 (comment)).

lizan · 2020-10-14T00:21:40Z

Yeah given the test situation in this repo I think it's fine as long as we will test this in Envoy or other wasm hosts.

PiotrSikora · 2020-10-14T02:03:19Z

Fix the bug in using a samewasm file for multiple times with a same VM ID:

on_context_create_ is invoked only for the first root context (this is because on_context_create_ is executed in onStart)

onConfigure is executed for the other root contexts without invoking on_context_create_,

As far as I know, this results in panics in Rust and TinyGo SDKs, and is not the intended behavior.

Isn't the real issue here that onCreate() isn't called before onConfigure() in the host code? Could you look at how feasible is fixing that instead of adding this workaround in onConfigure()?

mathetake · 2020-10-14T04:31:49Z

Isn't the real issue here that onCreate() isn't called before onConfigure() in the host code?

I just quick looked over the Envoy's implementation, but onConfigure is not directly executed there. IIUC, instead, it is executed deeply inside of the getOrCreateThreadLocalWasm implemented in this proxy-wasm-cpp-host repo.

mathetake · 2020-10-14T04:37:48Z

but yeah it seems natural to me to allow host implementations to determine when to execute onStart and onConfigure instead of implicitly invoking them during the VM creation/getter. I think it requires a little bit of refactoring here

PiotrSikora · 2020-10-16T05:02:51Z

Let's merge this as-is for now, since it fixes a crash, and we can do refactoring in a separate PR.

mathetake · 2020-10-16T05:06:38Z

Let's merge this as-is for now, since it fixes a crash, and we can do refactoring in a separate PR.

agreed

src/context.cc

Signed-off-by: mathetake <takeshi@tetrate.io>

src/context.cc

mathetake · 2020-10-16T08:51:38Z

@PiotrSikora I verified this change resolves the original issue and works fine with my local tests without crashing (Hopefully we will test them in this repository's CI in the future)

PiotrSikora · 2020-10-16T09:03:57Z

Thanks! Could you pull those changes into Envoy?

mathetake · 2020-10-16T09:04:53Z

OK, will do!

@PiotrSikora

The PR updates proxy-wasm-cpp-host dependency for enhancing the capability and fixing a bug in WASM extensions. The change consists of three PRs in proxy-wasm-cpp-host: 1. proxy-wasm/proxy-wasm-cpp-host#68 @PiotrSikora 2. proxy-wasm/proxy-wasm-cpp-host#65 @mathetake (me) 3. proxy-wasm/proxy-wasm-cpp-host#64 @mathetake (me) The code change can be found at proxy-wasm/proxy-wasm-cpp-host@49ed20e...c5658d3 . 1 & 2 enhance WASM capability, and 3 fixes a bug in situations where users share vm_id for multiple filters. For details, please take a look at these original PRs. Signed-off-by: mathetake <takeshi@tetrate.io>

@PiotrSikora

The PR updates proxy-wasm-cpp-host dependency for enhancing the capability and fixing a bug in WASM extensions. The change consists of three PRs in proxy-wasm-cpp-host: 1. proxy-wasm/proxy-wasm-cpp-host#68 @PiotrSikora 2. proxy-wasm/proxy-wasm-cpp-host#65 @mathetake (me) 3. proxy-wasm/proxy-wasm-cpp-host#64 @mathetake (me) The code change can be found at proxy-wasm/proxy-wasm-cpp-host@49ed20e...c5658d3 . 1 & 2 enhance WASM capability, and 3 fixes a bug in situations where users share vm_id for multiple filters. For details, please take a look at these original PRs. Signed-off-by: mathetake <takeshi@tetrate.io> Signed-off-by: Piotr Sikora <piotrsikora@google.com>

@PiotrSikora

* build: update rules_rust to allow Rustc in RBE (envoyproxy#13595) Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Piotr Sikora <piotrsikora@google.com> * fix macos v8 build (envoyproxy#13572) Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * wasm: update proxy-wasm-cpp-host (envoyproxy#13606) The PR updates proxy-wasm-cpp-host dependency for enhancing the capability and fixing a bug in WASM extensions. The change consists of three PRs in proxy-wasm-cpp-host: 1. proxy-wasm/proxy-wasm-cpp-host#68 @PiotrSikora 2. proxy-wasm/proxy-wasm-cpp-host#65 @mathetake (me) 3. proxy-wasm/proxy-wasm-cpp-host#64 @mathetake (me) The code change can be found at proxy-wasm/proxy-wasm-cpp-host@49ed20e...c5658d3 . 1 & 2 enhance WASM capability, and 3 fixes a bug in situations where users share vm_id for multiple filters. For details, please take a look at these original PRs. Signed-off-by: mathetake <takeshi@tetrate.io> Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: re-enable tests with precompiled modules. (envoyproxy#13583) Fixes envoyproxy#12335. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: flip the meaning of the "repository" in envoy_wasm_cc_binary(). (envoyproxy#13621) Change the meaning of the "repository" parameter to refer to an external Bazel repository, instead of using "@envoy" in targets that are included in the Envoy repository. This aligns with other envoy_* rules. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * build: support ppc64le with wasm (envoyproxy#13657) The build has only been tested with gn git sha 5da62d5 as recommended by ppc64 maintainers of the v8 runtime. Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com> * wasm: remove no longer needed Emscripten metadata. (envoyproxy#13667) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * fix wasm compilation (envoyproxy#13765) Signed-off-by: Asra Ali <asraa@google.com> * wasm: strip only Custom Sections with precompiled Wasm modules. (envoyproxy#13775) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * build: don't build shared libraries for zlib and zlib-ng. (envoyproxy#13652) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: update V8 to v8.7.220.10. (envoyproxy#13568) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * build: exclude wee8/out from inputs (envoyproxy#13866) If you build without sandboxing for performance, the output files from this custom build genrule contained timestamps which caused it to rebuild every single build. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com> * tls: fix detection of the upstream connection close event. (envoyproxy#13858) Fixes envoyproxy#13856. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: Force stop iteration after local response is sent (envoyproxy#13930) Resolves envoyproxy#13857 ref: -proxy-wasm/proxy-wasm-rust-sdk#44 -proxy-wasm/proxy-wasm-cpp-host#88 -proxy-wasm/proxy-wasm-cpp-host#93 Signed-off-by: mathetake <takeshi@tetrate.io> Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: fix order of callbacks for paused requests. (envoyproxy#13840) Fixes proxy-wasm/proxy-wasm-rust-sdk#43. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * wasm: fix network leak (envoyproxy#13836) Signed-off-by: Kuat Yessenov <kuat@google.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Co-authored-by: Rama Chavali <rama.rao@salesforce.com> Co-authored-by: Takeshi Yoneda <yoneda.takeshi.md@alumni.tsukuba.ac.jp> Co-authored-by: cmluciano <cmluciano@us.ibm.com> Co-authored-by: asraa <asraa@google.com> Co-authored-by: Keith Smiley <keithbsmiley@gmail.com> Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io> Co-authored-by: Kuat <kyessenov@users.noreply.github.com>

@PiotrSikora

The PR updates proxy-wasm-cpp-host dependency for enhancing the capability and fixing a bug in WASM extensions. The change consists of three PRs in proxy-wasm-cpp-host: 1. proxy-wasm/proxy-wasm-cpp-host#68 @PiotrSikora 2. proxy-wasm/proxy-wasm-cpp-host#65 @mathetake (me) 3. proxy-wasm/proxy-wasm-cpp-host#64 @mathetake (me) The code change can be found at proxy-wasm/proxy-wasm-cpp-host@49ed20e...c5658d3 . 1 & 2 enhance WASM capability, and 3 fixes a bug in situations where users share vm_id for multiple filters. For details, please take a look at these original PRs. Signed-off-by: mathetake <takeshi@tetrate.io>

lizan reviewed Oct 6, 2020

View reviewed changes

mathetake changed the title ~~invoke on_context_create_ for multiple root contexts in the shared VM situation~~ invoke on_context_create_ for multiple root contexts with a same VM ID Oct 12, 2020

mathetake changed the title ~~invoke on_context_create_ for multiple root contexts with a same VM ID~~ invoke on_context_create_ for all root contexts with a same VM ID Oct 12, 2020

PiotrSikora requested changes Oct 16, 2020

View reviewed changes

src/context.cc Show resolved Hide resolved

mathetake added 2 commits October 16, 2020 14:21

invoke on_context_create_ for multiple root contexts

74c1d83

Signed-off-by: mathetake <takeshi@tetrate.io>

review: check vm failure first

bada27a

Signed-off-by: mathetake <takeshi@tetrate.io>

mathetake force-pushed the master branch from c96fdb1 to bada27a Compare October 16, 2020 05:54

mathetake commented Oct 16, 2020

View reviewed changes

src/context.cc Show resolved Hide resolved

PiotrSikora requested changes Oct 16, 2020

View reviewed changes

src/context.cc Show resolved Hide resolved

src/context.cc Show resolved Hide resolved

PiotrSikora approved these changes Oct 16, 2020

View reviewed changes

PiotrSikora merged commit c5658d3 into proxy-wasm:master Oct 16, 2020

mathetake mentioned this pull request Oct 16, 2020

wasm: update proxy-wasm-cpp-host: enhance WASM capability and fix a bug envoyproxy/envoy#13606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

invoke on_context_create_ for all root contexts with a same VM ID #64

invoke on_context_create_ for all root contexts with a same VM ID #64

mathetake commented Oct 5, 2020 •

edited

Loading

lizan left a comment

PiotrSikora commented Oct 9, 2020

mathetake commented Oct 11, 2020 •

edited

Loading

PiotrSikora commented Oct 13, 2020

PiotrSikora commented Oct 13, 2020

mathetake commented Oct 13, 2020 •

edited

Loading

lizan commented Oct 14, 2020

PiotrSikora commented Oct 14, 2020

mathetake commented Oct 14, 2020

mathetake commented Oct 14, 2020 •

edited

Loading

PiotrSikora commented Oct 16, 2020

mathetake commented Oct 16, 2020

mathetake commented Oct 16, 2020 •

edited

Loading

PiotrSikora commented Oct 16, 2020

mathetake commented Oct 16, 2020

invoke on_context_create_ for all root contexts with a same VM ID #64

invoke on_context_create_ for all root contexts with a same VM ID #64

Conversation

mathetake commented Oct 5, 2020 • edited Loading

lizan left a comment

Choose a reason for hiding this comment

PiotrSikora commented Oct 9, 2020

mathetake commented Oct 11, 2020 • edited Loading

PiotrSikora commented Oct 13, 2020

PiotrSikora commented Oct 13, 2020

mathetake commented Oct 13, 2020 • edited Loading

lizan commented Oct 14, 2020

PiotrSikora commented Oct 14, 2020

mathetake commented Oct 14, 2020

mathetake commented Oct 14, 2020 • edited Loading

PiotrSikora commented Oct 16, 2020

mathetake commented Oct 16, 2020

mathetake commented Oct 16, 2020 • edited Loading

PiotrSikora commented Oct 16, 2020

mathetake commented Oct 16, 2020

mathetake commented Oct 5, 2020 •

edited

Loading

mathetake commented Oct 11, 2020 •

edited

Loading

mathetake commented Oct 13, 2020 •

edited

Loading

mathetake commented Oct 14, 2020 •

edited

Loading

mathetake commented Oct 16, 2020 •

edited

Loading