From 5cf9df22bcc31fe5ff9b17709bdb3f6347ca52dd Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 3 Nov 2020 16:47:13 +0900 Subject: [PATCH 1/7] stop iteration Signed-off-by: mathetake --- include/proxy-wasm/context.h | 6 ++++++ src/context.cc | 27 +++++++++++++++++++-------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 375ac80d..32c5bc2e 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -170,6 +170,11 @@ 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 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 @@ -388,6 +393,7 @@ class ContextBase : public RootInterface, std::shared_ptr temp_plugin_; // Remove once ABI v0.1.0 is gone. bool in_vm_context_created_ = false; bool destroyed_ = false; + bool stop_iteration_ = false; }; class DeferAfterCallActions { diff --git a/src/context.cc b/src/context.cc index a90db849..928de121 100644 --- a/src/context.cc +++ b/src/context.cc @@ -476,8 +476,12 @@ FilterHeadersStatus ContextBase::onRequestHeaders(uint32_t headers, bool end_of_ ->on_request_headers_abi_02_(this, id_, headers, static_cast(end_of_stream)) .u64_; - if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) + + if (stop_iteration_) { + return FilterHeadersStatus::StopIteration; + } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; + } return static_cast(result); } @@ -486,7 +490,7 @@ FilterDataStatus ContextBase::onRequestBody(uint32_t data_length, bool end_of_st DeferAfterCallActions actions(this); auto result = wasm_->on_request_body_(this, id_, data_length, static_cast(end_of_stream)).u64_; - if (result > static_cast(FilterDataStatus::StopIterationNoBuffer)) + if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) return FilterDataStatus::StopIterationNoBuffer; return static_cast(result); } @@ -495,8 +499,9 @@ FilterTrailersStatus ContextBase::onRequestTrailers(uint32_t trailers) { CHECK_HTTP(on_request_trailers_, FilterTrailersStatus::Continue, FilterTrailersStatus::StopIteration); DeferAfterCallActions actions(this); - if (static_cast(wasm_->on_request_trailers_(this, id_, trailers).u64_) == - FilterTrailersStatus::Continue) { + if (!stop_iteration_ && + static_cast(wasm_->on_request_trailers_(this, id_, trailers).u64_) == + FilterTrailersStatus::Continue) { return FilterTrailersStatus::Continue; } return FilterTrailersStatus::StopIteration; @@ -522,8 +527,12 @@ FilterHeadersStatus ContextBase::onResponseHeaders(uint32_t headers, bool end_of ->on_response_headers_abi_02_(this, id_, headers, static_cast(end_of_stream)) .u64_; - if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) + + if (stop_iteration_) { + return FilterHeadersStatus::StopIteration; + } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; + } return static_cast(result); } @@ -533,7 +542,8 @@ FilterDataStatus ContextBase::onResponseBody(uint32_t body_length, bool end_of_s DeferAfterCallActions actions(this); auto result = wasm_->on_response_body_(this, id_, body_length, static_cast(end_of_stream)).u64_; - if (result > static_cast(FilterDataStatus::StopIterationNoBuffer)) + + if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) return FilterDataStatus::StopIterationNoBuffer; return static_cast(result); } @@ -542,8 +552,9 @@ FilterTrailersStatus ContextBase::onResponseTrailers(uint32_t trailers) { CHECK_HTTP(on_response_trailers_, FilterTrailersStatus::Continue, FilterTrailersStatus::StopIteration); DeferAfterCallActions actions(this); - if (static_cast(wasm_->on_response_trailers_(this, id_, trailers).u64_) == - FilterTrailersStatus::Continue) { + if (!stop_iteration_ && + static_cast(wasm_->on_response_trailers_(this, id_, trailers).u64_) == + FilterTrailersStatus::Continue) { return FilterTrailersStatus::Continue; } return FilterTrailersStatus::StopIteration; From 9632caa6fa399006177e8916d4e65ef2dba28057 Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 3 Nov 2020 17:14:20 +0900 Subject: [PATCH 2/7] call context->stopIteration in send_local_response Signed-off-by: mathetake --- src/exports.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/exports.cc b/src/exports.cc index cda12514..4c31a75e 100644 --- a/src/exports.cc +++ b/src/exports.cc @@ -186,6 +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(); return WasmResult::Ok; } From c2b71cc8d98120f0ae1e2d2b33225eb2a00ce426 Mon Sep 17 00:00:00 2001 From: mathetake Date: Tue, 3 Nov 2020 23:59:07 +0900 Subject: [PATCH 3/7] reset stop_iteration_ in each callback Signed-off-by: mathetake --- src/context.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/context.cc b/src/context.cc index 928de121..1ea76d09 100644 --- a/src/context.cc +++ b/src/context.cc @@ -478,6 +478,7 @@ FilterHeadersStatus ContextBase::onRequestHeaders(uint32_t headers, bool end_of_ .u64_; if (stop_iteration_) { + stop_iteration_ = false; return FilterHeadersStatus::StopIteration; } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; @@ -490,8 +491,11 @@ FilterDataStatus ContextBase::onRequestBody(uint32_t data_length, bool end_of_st DeferAfterCallActions actions(this); auto result = wasm_->on_request_body_(this, id_, data_length, static_cast(end_of_stream)).u64_; - if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) + if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { + stop_iteration_ = false; return FilterDataStatus::StopIterationNoBuffer; + } + return static_cast(result); } @@ -504,6 +508,7 @@ FilterTrailersStatus ContextBase::onRequestTrailers(uint32_t trailers) { FilterTrailersStatus::Continue) { return FilterTrailersStatus::Continue; } + stop_iteration_ = false; return FilterTrailersStatus::StopIteration; } @@ -529,6 +534,7 @@ FilterHeadersStatus ContextBase::onResponseHeaders(uint32_t headers, bool end_of .u64_; if (stop_iteration_) { + stop_iteration_ = false; return FilterHeadersStatus::StopIteration; } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; @@ -543,8 +549,10 @@ FilterDataStatus ContextBase::onResponseBody(uint32_t body_length, bool end_of_s auto result = wasm_->on_response_body_(this, id_, body_length, static_cast(end_of_stream)).u64_; - if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) + if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { + stop_iteration_ = false; return FilterDataStatus::StopIterationNoBuffer; + } return static_cast(result); } @@ -557,6 +565,7 @@ FilterTrailersStatus ContextBase::onResponseTrailers(uint32_t trailers) { FilterTrailersStatus::Continue) { return FilterTrailersStatus::Continue; } + stop_iteration_ = false; return FilterTrailersStatus::StopIteration; } From 15f8b59ecbcf1635d9c72c5e0cdb5ebb96d41fba Mon Sep 17 00:00:00 2001 From: mathetake Date: Wed, 4 Nov 2020 10:19:42 +0900 Subject: [PATCH 4/7] move status conversion logic to helper methods Signed-off-by: mathetake --- include/proxy-wasm/context.h | 7 ++ src/context.cc | 128 ++++++++++++++++------------------- 2 files changed, 67 insertions(+), 68 deletions(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 32c5bc2e..8676b290 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -394,6 +394,13 @@ class ContextBase : public RootInterface, bool in_vm_context_created_ = false; bool destroyed_ = false; bool stop_iteration_ = false; + +private: + // helper functions + FilterHeadersStatus convertVmCallResultToFilterHeadersStatus(uint64_t result); + FilterDataStatus convertVmCallResultToFilterDataStatus(uint64_t result); + FilterTrailersStatus convertVmCallResultToFilterTrailersStatus(uint64_t result); + FilterMetadataStatus convertVmCallResultToFilterMetadataStatus(uint64_t result); }; class DeferAfterCallActions { diff --git a/src/context.cc b/src/context.cc index 1ea76d09..63a2f180 100644 --- a/src/context.cc +++ b/src/context.cc @@ -470,113 +470,71 @@ FilterHeadersStatus ContextBase::onRequestHeaders(uint32_t headers, bool end_of_ CHECK_HTTP2(on_request_headers_abi_01_, on_request_headers_abi_02_, FilterHeadersStatus::Continue, FilterHeadersStatus::StopIteration); DeferAfterCallActions actions(this); - auto result = wasm_->on_request_headers_abi_01_ - ? wasm_->on_request_headers_abi_01_(this, id_, headers).u64_ - : wasm_ - ->on_request_headers_abi_02_(this, id_, headers, - static_cast(end_of_stream)) - .u64_; - - if (stop_iteration_) { - stop_iteration_ = false; - return FilterHeadersStatus::StopIteration; - } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { - return FilterHeadersStatus::StopAllIterationAndWatermark; - } - return static_cast(result); + return convertVmCallResultToFilterHeadersStatus( + wasm_->on_request_headers_abi_01_ + ? wasm_->on_request_headers_abi_01_(this, id_, headers).u64_ + : wasm_ + ->on_request_headers_abi_02_(this, id_, headers, + static_cast(end_of_stream)) + .u64_); } FilterDataStatus ContextBase::onRequestBody(uint32_t data_length, bool end_of_stream) { CHECK_HTTP(on_request_body_, FilterDataStatus::Continue, FilterDataStatus::StopIterationNoBuffer); DeferAfterCallActions actions(this); - auto result = - wasm_->on_request_body_(this, id_, data_length, static_cast(end_of_stream)).u64_; - if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { - stop_iteration_ = false; - return FilterDataStatus::StopIterationNoBuffer; - } - - return static_cast(result); + return convertVmCallResultToFilterDataStatus( + wasm_->on_request_body_(this, id_, data_length, static_cast(end_of_stream)).u64_); } FilterTrailersStatus ContextBase::onRequestTrailers(uint32_t trailers) { CHECK_HTTP(on_request_trailers_, FilterTrailersStatus::Continue, FilterTrailersStatus::StopIteration); DeferAfterCallActions actions(this); - if (!stop_iteration_ && - static_cast(wasm_->on_request_trailers_(this, id_, trailers).u64_) == - FilterTrailersStatus::Continue) { - return FilterTrailersStatus::Continue; - } - stop_iteration_ = false; - return FilterTrailersStatus::StopIteration; + return convertVmCallResultToFilterTrailersStatus( + wasm_->on_request_trailers_(this, id_, trailers).u64_); } FilterMetadataStatus ContextBase::onRequestMetadata(uint32_t elements) { CHECK_HTTP(on_request_metadata_, FilterMetadataStatus::Continue, FilterMetadataStatus::Continue); DeferAfterCallActions actions(this); - if (static_cast(wasm_->on_request_metadata_(this, id_, elements).u64_) == - FilterMetadataStatus::Continue) { - return FilterMetadataStatus::Continue; - } - return FilterMetadataStatus::Continue; // This is currently the only return code. + return convertVmCallResultToFilterMetadataStatus( + wasm_->on_request_metadata_(this, id_, elements).u64_); } FilterHeadersStatus ContextBase::onResponseHeaders(uint32_t headers, bool end_of_stream) { CHECK_HTTP2(on_response_headers_abi_01_, on_response_headers_abi_02_, FilterHeadersStatus::Continue, FilterHeadersStatus::StopIteration); DeferAfterCallActions actions(this); - auto result = wasm_->on_response_headers_abi_01_ - ? wasm_->on_response_headers_abi_01_(this, id_, headers).u64_ - : wasm_ - ->on_response_headers_abi_02_(this, id_, headers, - static_cast(end_of_stream)) - .u64_; - - if (stop_iteration_) { - stop_iteration_ = false; - return FilterHeadersStatus::StopIteration; - } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { - return FilterHeadersStatus::StopAllIterationAndWatermark; - } - return static_cast(result); + return convertVmCallResultToFilterHeadersStatus( + wasm_->on_response_headers_abi_01_ + ? wasm_->on_response_headers_abi_01_(this, id_, headers).u64_ + : wasm_ + ->on_response_headers_abi_02_(this, id_, headers, + static_cast(end_of_stream)) + .u64_); } FilterDataStatus ContextBase::onResponseBody(uint32_t body_length, bool end_of_stream) { CHECK_HTTP(on_response_body_, FilterDataStatus::Continue, FilterDataStatus::StopIterationNoBuffer); DeferAfterCallActions actions(this); - auto result = - wasm_->on_response_body_(this, id_, body_length, static_cast(end_of_stream)).u64_; - - if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { - stop_iteration_ = false; - return FilterDataStatus::StopIterationNoBuffer; - } - return static_cast(result); + return convertVmCallResultToFilterDataStatus( + wasm_->on_response_body_(this, id_, body_length, static_cast(end_of_stream)).u64_); } FilterTrailersStatus ContextBase::onResponseTrailers(uint32_t trailers) { CHECK_HTTP(on_response_trailers_, FilterTrailersStatus::Continue, FilterTrailersStatus::StopIteration); DeferAfterCallActions actions(this); - if (!stop_iteration_ && - static_cast(wasm_->on_response_trailers_(this, id_, trailers).u64_) == - FilterTrailersStatus::Continue) { - return FilterTrailersStatus::Continue; - } - stop_iteration_ = false; - return FilterTrailersStatus::StopIteration; + return convertVmCallResultToFilterTrailersStatus( + wasm_->on_response_trailers_(this, id_, trailers).u64_); } FilterMetadataStatus ContextBase::onResponseMetadata(uint32_t elements) { CHECK_HTTP(on_response_metadata_, FilterMetadataStatus::Continue, FilterMetadataStatus::Continue); DeferAfterCallActions actions(this); - if (static_cast(wasm_->on_response_metadata_(this, id_, elements).u64_) == - FilterMetadataStatus::Continue) { - return FilterMetadataStatus::Continue; - } - return FilterMetadataStatus::Continue; // This is currently the only return code. + return convertVmCallResultToFilterMetadataStatus( + wasm_->on_response_metadata_(this, id_, elements).u64_); } void ContextBase::onHttpCallResponse(uint32_t token, uint32_t headers, uint32_t body_size, @@ -656,6 +614,40 @@ WasmResult ContextBase::setTimerPeriod(std::chrono::milliseconds period, return WasmResult::Ok; } +FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64_t result) { + if (stop_iteration_) { + stop_iteration_ = false; + return FilterHeadersStatus::StopIteration; + } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { + return FilterHeadersStatus::StopAllIterationAndWatermark; + } + return static_cast(result); +} + +FilterDataStatus ContextBase::convertVmCallResultToFilterDataStatus(uint64_t result) { + if (stop_iteration_ || result > static_cast(FilterDataStatus::StopIterationNoBuffer)) { + stop_iteration_ = false; + return FilterDataStatus::StopIterationNoBuffer; + } + return static_cast(result); +} + +FilterTrailersStatus ContextBase::convertVmCallResultToFilterTrailersStatus(uint64_t result) { + if (!stop_iteration_ && + static_cast(result) == FilterTrailersStatus::Continue) { + return FilterTrailersStatus::Continue; + } + stop_iteration_ = false; + return FilterTrailersStatus::StopIteration; +} + +FilterMetadataStatus ContextBase::convertVmCallResultToFilterMetadataStatus(uint64_t result) { + if (static_cast(result) == FilterMetadataStatus::Continue) { + return FilterMetadataStatus::Continue; + } + return FilterMetadataStatus::Continue; // This is currently the only return code. +} + ContextBase::~ContextBase() { // Do not remove vm context which has the same lifetime as wasm_. if (id_) { From 60161cd964c3c05f3f56b2c80f0858b3c7ba295b Mon Sep 17 00:00:00 2001 From: mathetake Date: Wed, 4 Nov 2020 11:04:03 +0900 Subject: [PATCH 5/7] review: style,return StopAllIterationAndWatermark for headerStatus if stop_iteration_ Signed-off-by: mathetake --- src/context.cc | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/context.cc b/src/context.cc index 63a2f180..09b0b4e7 100644 --- a/src/context.cc +++ b/src/context.cc @@ -615,10 +615,9 @@ WasmResult ContextBase::setTimerPeriod(std::chrono::milliseconds period, } FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64_t result) { - if (stop_iteration_) { + if (stop_iteration_ || + result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { stop_iteration_ = false; - return FilterHeadersStatus::StopIteration; - } else if (result > static_cast(FilterHeadersStatus::StopAllIterationAndWatermark)) { return FilterHeadersStatus::StopAllIterationAndWatermark; } return static_cast(result); @@ -633,12 +632,11 @@ FilterDataStatus ContextBase::convertVmCallResultToFilterDataStatus(uint64_t res } FilterTrailersStatus ContextBase::convertVmCallResultToFilterTrailersStatus(uint64_t result) { - if (!stop_iteration_ && - static_cast(result) == FilterTrailersStatus::Continue) { - return FilterTrailersStatus::Continue; + if (stop_iteration_ || result > static_cast(FilterTrailersStatus::StopIteration)) { + stop_iteration_ = false; + return FilterTrailersStatus::StopIteration; } - stop_iteration_ = false; - return FilterTrailersStatus::StopIteration; + return static_cast(result); } FilterMetadataStatus ContextBase::convertVmCallResultToFilterMetadataStatus(uint64_t result) { From a2af19a11deb9fd2bdbe0824db0b8aee53f37c27 Mon Sep 17 00:00:00 2001 From: mathetake Date: Wed, 4 Nov 2020 18:03:43 +0900 Subject: [PATCH 6/7] review: WASM extensions -> Proxy-wasm extensions Signed-off-by: mathetake --- include/proxy-wasm/context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 8676b290..660d83e4 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -171,7 +171,7 @@ class ContextBase : public RootInterface, virtual void destroy(); // Called to raise the flag which indicates that the context should stop iteration regardless of - // returned filter status from WASM extensions. For example, we ignore + // 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; }; From fbbdc549e08f9f309750eb271974e009ff2ac69e Mon Sep 17 00:00:00 2001 From: mathetake Date: Wed, 4 Nov 2020 18:06:58 +0900 Subject: [PATCH 7/7] review: Proxy-wasm -> Proxy-Wasm Signed-off-by: mathetake --- include/proxy-wasm/context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/proxy-wasm/context.h b/include/proxy-wasm/context.h index 660d83e4..68e5f2d1 100644 --- a/include/proxy-wasm/context.h +++ b/include/proxy-wasm/context.h @@ -171,7 +171,7 @@ class ContextBase : public RootInterface, 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 + // 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; };