From b416250e592543e25abe5afe29b88bc49ec6822a Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 10 Nov 2020 13:40:30 +0900 Subject: [PATCH 1/5] Clear stop_iteration_ flag non-status-returning callbacks. Signed-off-by: mathetake --- include/proxy-wasm/context.h | 6 ++++-- src/context.cc | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 03657a1b..2dc4ae96 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -377,6 +377,7 @@ class ContextBase : public RootInterface, protected: friend class WasmBase; + friend class DeferAfterCallActions; void initializeRootBase(WasmBase *wasm, std::shared_ptr plugin); std::string makeRootLogPrefix(std::string_view vm_id) const; @@ -402,11 +403,12 @@ class ContextBase : public RootInterface, class DeferAfterCallActions { public: - DeferAfterCallActions(ContextBase *context) : wasm_(context->wasm()) {} + DeferAfterCallActions(ContextBase *context) : context_(context) {} ~DeferAfterCallActions(); +protected: private: - WasmBase *const wasm_; + ContextBase *const context_; }; uint32_t resolveQueueForTest(std::string_view vm_id, std::string_view queue_name); diff --git a/src/context.cc b/src/context.cc index 6ab4b5e0..a9d12fad 100644 --- a/src/context.cc +++ b/src/context.cc @@ -225,7 +225,10 @@ SharedData global_shared_data; } // namespace -DeferAfterCallActions::~DeferAfterCallActions() { wasm_->doAfterVmCallActions(); } +DeferAfterCallActions::~DeferAfterCallActions() { + context_->stop_iteration_ = false; + context_->wasm()->doAfterVmCallActions(); +} WasmResult BufferBase::copyTo(WasmBase *wasm, size_t start, size_t length, uint64_t ptr_ptr, uint64_t size_ptr) const { From f249f053a172d96547be18e66ee121abc6e5baad Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 10 Nov 2020 13:51:29 +0900 Subject: [PATCH 2/5] Remove unnecessary stop_iteration_ = false Signed-off-by: mathetake --- src/context.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/context.cc b/src/context.cc index a9d12fad..01586cbf 100644 --- a/src/context.cc +++ b/src/context.cc @@ -627,7 +627,6 @@ WasmResult ContextBase::setTimerPeriod(std::chrono::milliseconds period, FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64_t result) { if (stop_iteration_ || result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { - stop_iteration_ = false; return FilterHeadersStatus::StopAllIterationAndWatermark; } return static_cast(result); @@ -635,7 +634,6 @@ FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64 FilterDataStatus ContextBase::convertVmCallResultToFilterDataStatus(uint64_t result) { if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { - stop_iteration_ = false; return FilterDataStatus::StopIterationNoBuffer; } return static_cast(result); @@ -643,7 +641,6 @@ FilterDataStatus ContextBase::convertVmCallResultToFilterDataStatus(uint64_t res FilterTrailersStatus ContextBase::convertVmCallResultToFilterTrailersStatus(uint64_t result) { if (stop_iteration_ || result > static_cast(FilterTrailersStatus::StopIteration)) { - stop_iteration_ = false; return FilterTrailersStatus::StopIteration; } return static_cast(result); From 8e22181b79623c57b1e9d001dcc6f02895bc45f1 Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 10 Nov 2020 14:11:48 +0900 Subject: [PATCH 3/5] review: remove unnecessary protected: Signed-off-by: mathetake --- include/proxy-wasm/context.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 2dc4ae96..c0c53e89 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -406,7 +406,6 @@ class DeferAfterCallActions { DeferAfterCallActions(ContextBase *context) : context_(context) {} ~DeferAfterCallActions(); -protected: private: ContextBase *const context_; }; From fa1c9c442fc92bd93b4dbd8674da60e1bfda4e7f Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 10 Nov 2020 07:54:38 +0000 Subject: [PATCH 4/5] review: StopIteration across Contexts. Signed-off-by: Piotr Sikora --- include/proxy-wasm/context.h | 6 ------ include/proxy-wasm/wasm.h | 7 +++++++ src/context.cc | 10 ++++++---- src/exports.cc | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index c0c53e89..d703b3ae 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -167,11 +167,6 @@ class ContextBase : public RootInterface, // Called before deleting the context. virtual void destroy(); - // Called to raise the flag which indicates that the context should stop iteration regardless of - // returned filter status from Proxy-Wasm extensions. For example, we ignore - // FilterHeadersStatus::Continue after a local reponse is sent by the host. - void stopIteration() { stop_iteration_ = true; }; - /** * Calls into the VM. * These are implemented by the proxy-independent host code. They are virtual to support some @@ -391,7 +386,6 @@ class ContextBase : public RootInterface, std::shared_ptr plugin_; bool in_vm_context_created_ = false; bool destroyed_ = false; - bool stop_iteration_ = false; private: // helper functions diff --git a/include/proxy-wasm/wasm.h b/include/proxy-wasm/wasm.h index b2c694db..c71c5848 100644 --- a/include/proxy-wasm/wasm.h +++ b/include/proxy-wasm/wasm.h @@ -122,6 +122,12 @@ class WasmBase : public std::enable_shared_from_this { AbiVersion abiVersion() { return abi_version_; } + // Called to raise the flag which indicates that the context should stop iteration regardless of + // returned filter status from Proxy-Wasm extensions. For example, we ignore + // FilterHeadersStatus::Continue after a local reponse is sent by the host. + void stopNextIteration(bool stop) { stop_iteration_ = stop; }; + bool isNextIterationStopped() { return stop_iteration_; }; + void addAfterVmCallAction(std::function f) { after_vm_call_actions_.push_back(f); } void doAfterVmCallActions() { // NB: this may be deleted by a delayed function unless prevented. @@ -223,6 +229,7 @@ class WasmBase : public std::enable_shared_from_this { std::string code_; std::string vm_configuration_; bool allow_precompiled_ = false; + bool stop_iteration_ = false; FailState failed_ = FailState::Ok; // Wasm VM fatal error. // ABI version. diff --git a/src/context.cc b/src/context.cc index 01586cbf..dc9b0637 100644 --- a/src/context.cc +++ b/src/context.cc @@ -226,7 +226,7 @@ SharedData global_shared_data; } // namespace DeferAfterCallActions::~DeferAfterCallActions() { - context_->stop_iteration_ = false; + context_->wasm()->stopNextIteration(false); context_->wasm()->doAfterVmCallActions(); } @@ -625,7 +625,7 @@ WasmResult ContextBase::setTimerPeriod(std::chrono::milliseconds period, } FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64_t result) { - if (stop_iteration_ || + if (wasm()->isNextIterationStopped() || result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; } @@ -633,14 +633,16 @@ FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64 } FilterDataStatus ContextBase::convertVmCallResultToFilterDataStatus(uint64_t result) { - if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { + if (wasm()->isNextIterationStopped() || + result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { return FilterDataStatus::StopIterationNoBuffer; } return static_cast(result); } FilterTrailersStatus ContextBase::convertVmCallResultToFilterTrailersStatus(uint64_t result) { - if (stop_iteration_ || result > static_cast(FilterTrailersStatus::StopIteration)) { + if (wasm()->isNextIterationStopped() || + result > static_cast(FilterTrailersStatus::StopIteration)) { return FilterTrailersStatus::StopIteration; } return static_cast(result); diff --git a/src/exports.cc b/src/exports.cc index 4c31a75e..1ffee322 100644 --- a/src/exports.cc +++ b/src/exports.cc @@ -186,7 +186,7 @@ Word send_local_response(void *raw_context, Word response_code, Word response_co auto additional_headers = toPairs(additional_response_header_pairs.value()); context->sendLocalResponse(response_code, body.value(), std::move(additional_headers), grpc_code, details.value()); - context->stopIteration(); + context->wasm()->stopNextIteration(true); return WasmResult::Ok; } From 70cfd06b115f2677042059845d162ffe9e7a1ea8 Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 10 Nov 2020 17:11:12 +0900 Subject: [PATCH 5/5] review: remove unnecessary changes on DeferAfterCallActions Signed-off-by: mathetake --- include/proxy-wasm/context.h | 5 ++--- src/context.cc | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index d703b3ae..2313d9cd 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -372,7 +372,6 @@ class ContextBase : public RootInterface, protected: friend class WasmBase; - friend class DeferAfterCallActions; void initializeRootBase(WasmBase *wasm, std::shared_ptr plugin); std::string makeRootLogPrefix(std::string_view vm_id) const; @@ -397,11 +396,11 @@ class ContextBase : public RootInterface, class DeferAfterCallActions { public: - DeferAfterCallActions(ContextBase *context) : context_(context) {} + DeferAfterCallActions(ContextBase *context) : wasm_(context->wasm()) {} ~DeferAfterCallActions(); private: - ContextBase *const context_; + WasmBase *const wasm_; }; uint32_t resolveQueueForTest(std::string_view vm_id, std::string_view queue_name); diff --git a/src/context.cc b/src/context.cc index dc9b0637..89daa97a 100644 --- a/src/context.cc +++ b/src/context.cc @@ -226,8 +226,8 @@ SharedData global_shared_data; } // namespace DeferAfterCallActions::~DeferAfterCallActions() { - context_->wasm()->stopNextIteration(false); - context_->wasm()->doAfterVmCallActions(); + wasm_->stopNextIteration(false); + wasm_->doAfterVmCallActions(); } WasmResult BufferBase::copyTo(WasmBase *wasm, size_t start, size_t length, uint64_t ptr_ptr,