Skip to content

wasm: allow execution of multiple instances of the same plugin. #13753

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

Merged
mattklein123 merged 26 commits into from
Nov 12, 2020
Merged

wasm: allow execution of multiple instances of the same plugin. #13753

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
697dc19
wasm: allow execution of multiple instances of the same plugin.
PiotrSikora Oct 25, 2020
672c1ff
review: fix clang-tidy.
PiotrSikora Oct 29, 2020
8d49c1d
wasm: remove the gap between rust and cpp test code for logging
mathetake Oct 30, 2020
9f98a40
review: use merged commit.
PiotrSikora Oct 30, 2020
60003c1
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Oct 30, 2020
6f3392a
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 2, 2020
33f8b48
review: mark destructors virtual.
PiotrSikora Nov 2, 2020
375b02d
reivew: use tls_slot_->currentThreadRegistered().
PiotrSikora Nov 2, 2020
8ad9cbd
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 3, 2020
3a89a49
review: use ThreadLocal::TypedSlot<WasmHandle>.
PiotrSikora Nov 3, 2020
2facb41
review: re-add currentThreadRegistered() check.
PiotrSikora Nov 3, 2020
4c0ed76
review: style.
PiotrSikora Nov 4, 2020
88d5dc0
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 4, 2020
40015d0
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 4, 2020
e94fd82
review: use OptRef<WasmHandle>.
PiotrSikora Nov 4, 2020
6d978fc
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 5, 2020
62a015e
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 6, 2020
487d930
review: add shutdown for a singleton Wasm Service.
PiotrSikora Nov 6, 2020
930282d
review: add comments.
PiotrSikora Nov 6, 2020
cd04f3b
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 9, 2020
9965dcd
review: use PluginHandle for shutdown-on-destroy semantics.
PiotrSikora Nov 9, 2020
f16ba7d
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 10, 2020
3d4e28e
review: point at merged commit.
PiotrSikora Nov 10, 2020
d128aaf
review: update release date.
PiotrSikora Nov 10, 2020
7bc7c9a
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 11, 2020
e15e498
Merge remote-tracking branch 'origin/master' into PiotrSikora/wasm_co…
PiotrSikora Nov 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,8 +830,8 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "WebAssembly for Proxies (C++ host implementation)",
project_desc = "WebAssembly for Proxies (C++ host implementation)",
project_url = "https://github.com/proxy-wasm/proxy-wasm-cpp-host",
version = "40fd3d03842c07d65fed907a6b6ed0f89d68d531",
sha256 = "b5ae746e66b6209ea0cce86d6c21de99dacbec1da9cdadd53a9ec46bc296a3ba",
version = "f08baacadbe656674414f0878e18852bed67b795",
sha256 = "dc3239c674d19198f71becf5d1c2bf159128af3c72ec2f9c532b3153ecb6d5a1",
strip_prefix = "proxy-wasm-cpp-host-{version}",
urls = ["https://github.com/proxy-wasm/proxy-wasm-cpp-host/archive/{version}.tar.gz"],
use_category = ["dataplane_ext"],
Expand All @@ -842,7 +842,7 @@ REPOSITORY_LOCATIONS_SPEC = dict(
"envoy.filters.network.wasm",
"envoy.stat_sinks.wasm",
],
release_date = "2020-10-27",
release_date = "2020-10-30",
cpe = "N/A",
),
emscripten_toolchain = dict(
Expand Down
25 changes: 6 additions & 19 deletions source/extensions/access_loggers/wasm/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con
const auto& config = MessageUtil::downcastAndValidate<
const envoy::extensions::access_loggers::wasm::v3::WasmAccessLog&>(
proto_config, context.messageValidationVisitor());
auto access_log =
std::make_shared<WasmAccessLog>(config.config().root_id(), nullptr, std::move(filter));

// Create a base WASM to verify that the code loads before setting/cloning the for the
// individual threads.
Expand All @@ -35,25 +33,14 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con
envoy::config::core::v3::TrafficDirection::UNSPECIFIED, context.localInfo(),
nullptr /* listener_metadata */);

auto callback = [access_log, &context, plugin](Common::Wasm::WasmHandleSharedPtr base_wasm) {
auto tls_slot = context.threadLocal().allocateSlot();
auto access_log = std::make_shared<WasmAccessLog>(plugin, nullptr, std::move(filter));

auto callback = [access_log, &context, plugin](Common::Wasm::WasmHandleSharedPtr base_wasm) {
// NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
tls_slot->set(
[base_wasm,
plugin](Event::Dispatcher& dispatcher) -> std::shared_ptr<ThreadLocal::ThreadLocalObject> {
if (!base_wasm) {
// There is no way to prevent the connection at this point. The user could choose to use
// an HTTP Wasm plugin and only handle onLog() which would correctly close the
// connection in onRequestHeaders().
if (!plugin->fail_open_) {
ENVOY_LOG(critical, "Plugin configured to fail closed failed to load");
}
return nullptr;
}
return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(
Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher));
});
auto tls_slot = ThreadLocal::TypedSlot<WasmHandle>::makeUnique(context.threadLocal());
tls_slot->set([base_wasm, plugin](Event::Dispatcher& dispatcher) {
return Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher);
});
access_log->setTlsSlot(std::move(tls_slot));
};

Expand Down
30 changes: 22 additions & 8 deletions source/extensions/access_loggers/wasm/wasm_access_log_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ namespace Extensions {
namespace AccessLoggers {
namespace Wasm {

using Envoy::Extensions::Common::Wasm::PluginSharedPtr;
using Envoy::Extensions::Common::Wasm::WasmHandle;

class WasmAccessLog : public AccessLog::Instance {
public:
WasmAccessLog(absl::string_view root_id, ThreadLocal::SlotPtr tls_slot,
WasmAccessLog(const PluginSharedPtr& plugin, ThreadLocal::TypedSlotPtr<WasmHandle>&& tls_slot,
AccessLog::FilterPtr filter)
: root_id_(root_id), tls_slot_(std::move(tls_slot)), filter_(std::move(filter)) {}
: plugin_(plugin), tls_slot_(std::move(tls_slot)), filter_(std::move(filter)) {}

void log(const Http::RequestHeaderMap* request_headers,
const Http::ResponseHeaderMap* response_headers,
const Http::ResponseTrailerMap* response_trailers,
Expand All @@ -30,20 +32,32 @@ class WasmAccessLog : public AccessLog::Instance {
}
}

if (tls_slot_->get()) {
tls_slot_->getTyped<WasmHandle>().wasm()->log(root_id_, request_headers, response_headers,
response_trailers, stream_info);
auto handle = tls_slot_->get();
if (handle.has_value()) {
handle->wasm()->log(plugin_, request_headers, response_headers, response_trailers,
stream_info);
}
}

void setTlsSlot(ThreadLocal::SlotPtr tls_slot) {
void setTlsSlot(ThreadLocal::TypedSlotPtr<WasmHandle>&& tls_slot) {
ASSERT(tls_slot_ == nullptr);
tls_slot_ = std::move(tls_slot);
}

~WasmAccessLog() override {
if (tls_slot_->currentThreadRegistered()) {

@mattklein123 mattklein123 Nov 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something about this seems broken. Why is this check here? On what thread is the WASM access log created and destroyed?

// Start graceful shutdown of Wasm plugin, unless Envoy is already shutting down.
tls_slot_->runOnAllThreads([plugin = plugin_](OptRef<WasmHandle> handle) {
if (handle.has_value()) {
handle->wasm()->startShutdown(plugin);
}
});
}
}

private:
std::string root_id_;
ThreadLocal::SlotPtr tls_slot_;
Common::Wasm::PluginSharedPtr plugin_;
ThreadLocal::TypedSlotPtr<WasmHandle> tls_slot_;
AccessLog::FilterPtr filter_;
};

Expand Down
11 changes: 5 additions & 6 deletions source/extensions/bootstrap/wasm/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,17 @@ void WasmFactory::createWasm(const envoy::extensions::wasm::v3::WasmService& con
}
if (singleton) {
// Return a Wasm VM which will be stored as a singleton by the Server.
cb(std::make_unique<WasmService>(
Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, context.dispatcher())));
cb(std::make_unique<WasmService>(plugin, Common::Wasm::getOrCreateThreadLocalWasm(
base_wasm, plugin, context.dispatcher())));
return;
}
// Per-thread WASM VM.
// NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
auto tls_slot = context.threadLocal().allocateSlot();
auto tls_slot = ThreadLocal::TypedSlot<WasmHandle>::makeUnique(context.threadLocal());
tls_slot->set([base_wasm, plugin](Event::Dispatcher& dispatcher) {
return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(
Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher));
return Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher);
});
cb(std::make_unique<WasmService>(std::move(tls_slot)));
cb(std::make_unique<WasmService>(plugin, std::move(tls_slot)));
};

if (!Common::Wasm::createWasm(
Expand Down
26 changes: 23 additions & 3 deletions source/extensions/bootstrap/wasm/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,34 @@ namespace Extensions {
namespace Bootstrap {
namespace Wasm {

using Envoy::Extensions::Common::Wasm::WasmHandle;

class WasmService {
public:
WasmService(Common::Wasm::WasmHandleSharedPtr singleton) : singleton_(std::move(singleton)) {}
WasmService(ThreadLocal::SlotPtr tls_slot) : tls_slot_(std::move(tls_slot)) {}
WasmService(Common::Wasm::PluginSharedPtr plugin, Common::Wasm::WasmHandleSharedPtr singleton)
: plugin_(plugin), singleton_(std::move(singleton)) {}
WasmService(Common::Wasm::PluginSharedPtr plugin,
ThreadLocal::TypedSlotPtr<WasmHandle>&& tls_slot)
: plugin_(plugin), tls_slot_(std::move(tls_slot)) {}

virtual ~WasmService() {
if (singleton_) {
// Singleton Wasm Service is running only on the main thread.
singleton_->wasm()->startShutdown(plugin_);
} else if (tls_slot_->currentThreadRegistered()) {
// Start graceful shutdown of Wasm plugin, unless Envoy is already shutting down.
tls_slot_->runOnAllThreads([plugin = plugin_](OptRef<WasmHandle> handle) {
if (handle.has_value()) {
handle->wasm()->startShutdown(plugin);
}
});
}
}

private:
Common::Wasm::PluginSharedPtr plugin_;
Common::Wasm::WasmHandleSharedPtr singleton_;
ThreadLocal::SlotPtr tls_slot_;
ThreadLocal::TypedSlotPtr<WasmHandle> tls_slot_;
};

using WasmServicePtr = std::unique_ptr<WasmService>;
Expand Down
12 changes: 6 additions & 6 deletions source/extensions/common/wasm/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,8 +810,8 @@ BufferInterface* Context::getBuffer(WasmBufferType type) {
case WasmBufferType::VmConfiguration:
return buffer_.set(wasm()->vm_configuration());
case WasmBufferType::PluginConfiguration:
if (plugin_) {
return buffer_.set(plugin_->plugin_configuration_);
if (temp_plugin_) {
return buffer_.set(temp_plugin_->plugin_configuration_);
}
return nullptr;
Comment thread
PiotrSikora marked this conversation as resolved.
case WasmBufferType::HttpRequestBody:
Expand Down Expand Up @@ -1182,18 +1182,18 @@ bool Context::validateConfiguration(absl::string_view configuration,
if (!wasm()->validate_configuration_) {
return true;
}
plugin_ = plugin_base;
temp_plugin_ = plugin_base;
auto result =
wasm()
->validate_configuration_(this, id_, static_cast<uint32_t>(configuration.size()))
.u64_ != 0;
plugin_.reset();
temp_plugin_.reset();
return result;
}

absl::string_view Context::getConfiguration() {
if (plugin_) {
return plugin_->plugin_configuration_;
if (temp_plugin_) {
return temp_plugin_->plugin_configuration_;
} else {
return wasm()->vm_configuration();
}
Expand Down
15 changes: 11 additions & 4 deletions source/extensions/common/wasm/wasm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,16 @@ ContextBase* Wasm::createRootContext(const std::shared_ptr<PluginBase>& plugin)

ContextBase* Wasm::createVmContext() { return new Context(this); }

void Wasm::log(absl::string_view root_id, const Http::RequestHeaderMap* request_headers,
void Wasm::log(const PluginSharedPtr& plugin, const Http::RequestHeaderMap* request_headers,
const Http::ResponseHeaderMap* response_headers,
const Http::ResponseTrailerMap* response_trailers,
const StreamInfo::StreamInfo& stream_info) {
auto context = getRootContext(root_id);
auto context = getRootContext(plugin, true);
context->log(request_headers, response_headers, response_trailers, stream_info);
}

void Wasm::onStatsUpdate(absl::string_view root_id, Envoy::Stats::MetricSnapshot& snapshot) {
auto context = getRootContext(root_id);
void Wasm::onStatsUpdate(const PluginSharedPtr& plugin, Envoy::Stats::MetricSnapshot& snapshot) {
auto context = getRootContext(plugin, true);
context->onStatsUpdate(snapshot);
}

Expand Down Expand Up @@ -478,6 +478,13 @@ WasmHandleSharedPtr getOrCreateThreadLocalWasm(const WasmHandleSharedPtr& base_w
const PluginSharedPtr& plugin,
Event::Dispatcher& dispatcher,
CreateContextFn create_root_context_for_testing) {
if (!base_wasm) {
if (!plugin->fail_open_) {
ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::wasm), critical,
"Plugin configured to fail closed failed to load");
}
return nullptr;
}
return std::static_pointer_cast<WasmHandle>(proxy_wasm::getOrCreateThreadLocalWasm(
std::static_pointer_cast<WasmHandle>(base_wasm), plugin,
getCloneFactory(getWasmExtension(), dispatcher, create_root_context_for_testing)));
Expand Down
9 changes: 5 additions & 4 deletions source/extensions/common/wasm/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ class Wasm : public WasmBase, Logger::Loggable<Logger::Id::wasm> {

Upstream::ClusterManager& clusterManager() const { return cluster_manager_; }
Event::Dispatcher& dispatcher() { return dispatcher_; }
Context* getRootContext(absl::string_view root_id) {
return static_cast<Context*>(WasmBase::getRootContext(root_id));
Context* getRootContext(const std::shared_ptr<PluginBase>& plugin, bool allow_closed) {
return static_cast<Context*>(WasmBase::getRootContext(plugin, allow_closed));
}
void setTimerPeriod(uint32_t root_context_id, std::chrono::milliseconds period) override;
virtual void tickHandler(uint32_t root_context_id);
Expand All @@ -72,12 +72,13 @@ class Wasm : public WasmBase, Logger::Loggable<Logger::Id::wasm> {
void getFunctions() override;

// AccessLog::Instance
void log(absl::string_view root_id, const Http::RequestHeaderMap* request_headers,
void log(const PluginSharedPtr& plugin, const Http::RequestHeaderMap* request_headers,
const Http::ResponseHeaderMap* response_headers,
const Http::ResponseTrailerMap* response_trailers,
const StreamInfo::StreamInfo& stream_info);

void onStatsUpdate(absl::string_view root_id, Envoy::Stats::MetricSnapshot& snapshot);
void onStatsUpdate(const PluginSharedPtr& plugin, Envoy::Stats::MetricSnapshot& snapshot);

virtual std::string buildVersion() { return BUILD_VERSION_NUMBER; }

void initializeLifecycle(Server::ServerLifecycleNotifier& lifecycle_notifier);
Expand Down
34 changes: 15 additions & 19 deletions source/extensions/filters/http/wasm/wasm_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,13 @@
#include "extensions/filters/http/wasm/wasm_filter.h"

#include "envoy/http/codes.h"

#include "common/buffer/buffer_impl.h"
#include "common/common/assert.h"
#include "common/common/enum_to_int.h"
#include "common/http/header_map_impl.h"
#include "common/http/message_impl.h"
#include "common/http/utility.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Wasm {

FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config,
Server::Configuration::FactoryContext& context)
: tls_slot_(context.threadLocal().allocateSlot()) {
: tls_slot_(ThreadLocal::TypedSlot<WasmHandle>::makeUnique(context.threadLocal())) {
plugin_ = std::make_shared<Common::Wasm::Plugin>(
config.config().name(), config.config().root_id(), config.config().vm_config().vm_id(),
config.config().vm_config().runtime(),
Expand All @@ -26,15 +17,9 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Was
auto plugin = plugin_;
auto callback = [plugin, this](const Common::Wasm::WasmHandleSharedPtr& base_wasm) {
// NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call.
tls_slot_->set(
[base_wasm,
plugin](Event::Dispatcher& dispatcher) -> std::shared_ptr<ThreadLocal::ThreadLocalObject> {
if (!base_wasm) {
return nullptr;
}
return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(
Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher));
});
tls_slot_->set([base_wasm, plugin](Event::Dispatcher& dispatcher) {
return Common::Wasm::getOrCreateThreadLocalWasm(base_wasm, plugin, dispatcher);
});
};

if (!Common::Wasm::createWasm(
Expand All @@ -46,6 +31,17 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Was
}
}

FilterConfig::~FilterConfig() {
if (tls_slot_->currentThreadRegistered()) {

@mattklein123 mattklein123 Nov 5, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same question.

@PiotrSikora PiotrSikora Nov 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's focus on this one:

At startup:

main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:26] XXX before createWasm
main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm
worker thread 1 -> [39577][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm
worker thread 2 -> [39578][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm

With config change at runtime:

main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:26] XXX before createWasm
main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm
worker thread 2 -> [39578][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm
worker thread 1 -> [39577][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:21] XXX before getOrCreateThreadLocalWasm
main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:37] XXX before if currentThreadRegistered
main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:40] XXX inside the runOnAllThreads callback
worker thread 1 -> [39577][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:40] XXX inside the runOnAllThreads callback
worker thread 2 -> [39578][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:40] XXX inside the runOnAllThreads callback

After /quitquitquit:

main thread     -> [39289][warning][wasm] [source/extensions/filters/http/wasm/wasm_filter.cc:37] XXX before if currentThreadRegistered
main thread     -> [39289][critical][assert] [source/common/thread_local/thread_local_impl.cc:150] assert failure: !shutdown_.

Assert is hit with tls_slot_->currentThreadRegistered() removed.

@PiotrSikora PiotrSikora Nov 5, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I've looked at all the other instances of runOnAllThreads() and it doesn't look that any of the other extensions use it for cleanup during shutdown. What's more, ThreadLocalStoreImpl specifically checks for !shutting_down_ before executing anything via runOnAllThreads(), so I don't think it's supposed to work during shutdown.

Should I add a comment to say that cleanup is skipped during shutdown? Or do you want me to make some other changes?

@PiotrSikora PiotrSikora Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are reports of assert failure: currentThreadRegisteredWorker(index). in the upcoming Istio release (based on Envoy 1.16.0), so perhaps there is a recent regression:

assert failure: currentThreadRegisteredWorker(index).
Caught Aborted, suspect faulting address 0x53900000012
Backtrace (use tools/stack_decode.py to get line numbers):
Envoy version: 211ca038383b61c9fb600758bf170aea5508800c/1.16.0/Clean/DEBUG/BoringSSL
 #0: Envoy::SignalAction::sigHandler() [0x5602aeb3f66c]
 #1: __restore_rt [0x7fbabc9518a0]
 #2: Envoy::ThreadLocal::InstanceImpl::SlotImpl::runOnAllThreads()::$_2::operator()() [0x5602ada1bffc]
 #3: std::__1::__invoke<>() [0x5602ada1bfad]
 #4: std::__1::__invoke_void_return_wrapper<>::__call<>() [0x5602ada1bf5d]
 #5: std::__1::__function::__alloc_func<>::operator()() [0x5602ada1bf2d]
 #6: std::__1::__function::__func<>::operator()() [0x5602ada1b14e]
 #7: std::__1::__function::__value_func<>::operator()() [0x5602a92053c5]
 #8: std::__1::function<>::operator()() [0x5602a9205385]
 #9: Envoy::ThreadLocal::InstanceImpl::SlotImpl::wrapCallback()::$_0::operator()() [0x5602ada19046]
#10: std::__1::__invoke<>() [0x5602ada18fed]
#11: std::__1::__invoke_void_return_wrapper<>::__call<>() [0x5602ada18f9d]
#12: std::__1::__function::__alloc_func<>::operator()() [0x5602ada18f6d]
#13: std::__1::__function::__func<>::operator()() [0x5602ada1814e]
#14: std::__1::__function::__value_func<>::operator()() [0x5602a92053c5]
#15: std::__1::function<>::operator()() [0x5602a9205385]
#16: Envoy::ThreadLocal::InstanceImpl::runOnAllThreads() [0x5602ada143f1]
#17: Envoy::ThreadLocal::InstanceImpl::SlotImpl::runOnAllThreads() [0x5602ada13a97]
#18: Envoy::Extensions::Clusters::Aggregate::Cluster::refresh() [0x5602a95817de]
...

(From: istio/istio#28620)

@jmarantz jmarantz Nov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RE cleanup during shutdown: why not just rely on the destructors for the objects held in the TLS slots?

@mattklein123 mattklein123 Nov 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, in the "early times" IIRC there used to be a shutdown() method for TLS objects that we would call to avoid this type of situation. Essentially we would give each TLS object a chance to clean itself up. I don't remember the history, but I removed it at some point because I determined all of the lifecycle stuff could be simplified because we destroy all TLS objects when shutdownThread() is called. So per @jmarantz why can't the TLS object constructor just do what it has to do when it's destroyed on the thread its owned by?

For the aggregate cluster issue, it's possible there is a regression from some of the ref counting changes, but I think this is unrelated. Can you or someone else open a different issue on that with a complete callstack and whether it's happening during shutdown or not?

@PiotrSikora PiotrSikora Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're doing graceful shutdown of the Proxy-Wasm plugins within the WasmVM, so we cannot destroy them in-place, but I think that if I hold onto thread-local object representing RootContext instead of shared Plugin, then I might be able to dispatch this without runOnAllThreads. Let me see what I can do.

// Start graceful shutdown of Wasm plugin, unless Envoy is already shutting down.
tls_slot_->runOnAllThreads([plugin = plugin_](OptRef<WasmHandle> handle) {
if (handle.has_value()) {
handle->wasm()->startShutdown(plugin);
}
});
}
}

} // namespace Wasm
} // namespace HttpFilters
} // namespace Extensions
Expand Down
10 changes: 6 additions & 4 deletions source/extensions/filters/http/wasm/wasm_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,27 @@ class FilterConfig : Logger::Loggable<Logger::Id::wasm> {
public:
FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& proto_config,
Server::Configuration::FactoryContext& context);
virtual ~FilterConfig();

std::shared_ptr<Context> createFilter() {
Wasm* wasm = nullptr;
if (tls_slot_->get()) {
wasm = tls_slot_->getTyped<WasmHandle>().wasm().get();
auto handle = tls_slot_->get();
if (handle.has_value()) {
wasm = handle->wasm().get();
}
if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
return nullptr;
}
if (wasm && !root_context_id_) {
root_context_id_ = wasm->getRootContext(plugin_->root_id_)->id();
root_context_id_ = wasm->getRootContext(plugin_, false)->id();
}
return std::make_shared<Context>(wasm, root_context_id_, plugin_);
}

private:
uint32_t root_context_id_{0};
Envoy::Extensions::Common::Wasm::PluginSharedPtr plugin_;
ThreadLocal::SlotPtr tls_slot_;
ThreadLocal::TypedSlotPtr<WasmHandle> tls_slot_;
Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;
};

Expand Down
Loading