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.

Sign up for GitHub

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

Jump to bottom

wasm: fix order of callbacks for paused requests. #13840

Merged
merged 6 commits into from
Nov 13, 2020
Merged

wasm: fix order of callbacks for paused requests. #13840

merged 6 commits into from
Nov 13, 2020

Conversation

PiotrSikora
Copy link
Contributor

Fixes proxy-wasm/proxy-wasm-rust-sdk#43.

Signed-off-by: Piotr Sikora piotrsikora@google.com

@PiotrSikora
wasm: defer continue processing to avoid reentrant calls.
33f6d8d 
Fixes proxy-wasm/proxy-wasm-rust-sdk#43.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora requested a review from mathetake October 30, 2020 22:57
@PiotrSikora PiotrSikora requested a review from lizan as a code owner October 30, 2020 22:57
@PiotrSikora PiotrSikora mentioned this pull request Oct 30, 2020
Closed
@mathetake
Copy link
Member

mathetake commented Nov 2, 2020 •
edited
Loading

looks fine but I just want to reproduce and verify this actually fixes that reported crash error

@PiotrSikora
Copy link
Contributor Author

looks fine but I just want to reproduce and verify this actually fixes that reported crash error

Agreed, hence the "waiting" label. I couldn't reproduce the crash using examples/http_auth_random.rs, which matches the description in the issue, and it works fine with and without this patch, so I'm waiting for the reporter to either provide reproducer or confirm that the fix is working.

lizan
lizan previously approved these changes Nov 2, 2020
View reviewed changes
@PiotrSikora PiotrSikora mentioned this pull request Nov 6, 2020
Merged
@antoniovicente
Copy link
Contributor

Would be good to have a repo of the issue and tests to cover the expected behavior.

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_re…
c8ad09a 
…sume
@PiotrSikora
review: add test.
38eba4d 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: always process HTTP headers before HTTP body.
684a217 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@repokitteh-read-only repokitteh-read-only bot added deps Approval required for changes to Envoy's external dependencies and removed waiting labels Nov 12, 2020
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #13840 was synchronize by PiotrSikora.

see: more, trace.

@PiotrSikora
Copy link
Contributor Author

Would be good to have a repo of the issue and tests to cover the expected behavior.

Added, PTAL.

@PiotrSikora PiotrSikora changed the title wasm: defer continue processing to avoid reentrant calls. wasm: fix order of callbacks for paused requests. Nov 12, 2020
@PiotrSikora
Copy link
Contributor Author

Note: This is ready for review, but #13753 needs to be merged first, because of order of changes in proxy-wasm-cpp-host.

mathetake
mathetake approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the added test covered the reported issue. LGTM.

I think it would be nice we have some sort of e2e WASM filter test infrastructure where we gather plugins as many as possible, and test them against actual running Envoy rather than the mock tests like the existing ones in Envoy repo. Given that users run any WASM binary in Envoy, the mock test seems not enough for us to protect against crashes.

@PiotrSikora
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_re…
4898c94 
…sume

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Contributor Author

OK, now that the other PR is merged and all tests passed, this is ready for review and/or merge. PTAL.

@PiotrSikora
Copy link
Contributor Author

@envoyproxy/dependency-shepherds friendly ping for approval.

@mattklein123 mattklein123 merged commit 4889735 into envoyproxy:master Nov 13, 2020
PiotrSikora added a commit to PiotrSikora/envoy that referenced this pull request Nov 20, 2020
@PiotrSikora
wasm: fix order of callbacks for paused requests. (envoyproxy#13840)
36a3e6f 
Fixes proxy-wasm/proxy-wasm-rust-sdk#43.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Nov 20, 2020
@PiotrSikora @lizan
@ramaraochavali @asraa @keith @mathetake @kyessenov
Proxy-Wasm mega backport (#284)
5892439 
* 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>
This was referenced Nov 20, 2020
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathetake mathetake mathetake approved these changes

@mattklein123 mattklein123 mattklein123 approved these changes

@lizan lizan lizan approved these changes

@antoniovicente antoniovicente Awaiting requested review from antoniovicente

Assignees

@lizan lizan

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Callbacks must not be invoked from within "resume_http_request()"
5 participants
@PiotrSikora @mathetake @antoniovicente @lizan @mattklein123