-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
Changes from 1 commit
697dc19
672c1ff
8d49c1d
9f98a40
60003c1
6f3392a
33f8b48
375b02d
8ad9cbd
3a89a49
2facb41
4c0ed76
88d5dc0
40015d0
e94fd82
6d978fc
62a015e
487d930
930282d
cd04f3b
9965dcd
f16ba7d
3d4e28e
d128aaf
7bc7c9a
e15e498
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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::SlotPtr 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, | ||
|
|
@@ -31,7 +33,7 @@ class WasmAccessLog : public AccessLog::Instance { | |
| } | ||
|
|
||
| if (tls_slot_->get()) { | ||
| tls_slot_->getTyped<WasmHandle>().wasm()->log(root_id_, request_headers, response_headers, | ||
| tls_slot_->getTyped<WasmHandle>().wasm()->log(plugin_, request_headers, response_headers, | ||
| response_trailers, stream_info); | ||
| } | ||
| } | ||
|
|
@@ -41,8 +43,18 @@ class WasmAccessLog : public AccessLog::Instance { | |
| tls_slot_ = std::move(tls_slot); | ||
| } | ||
|
|
||
| ~WasmAccessLog() { | ||
| if (tls_slot_->get()) { | ||
|
PiotrSikora marked this conversation as resolved.
Outdated
|
||
| tls_slot_->runOnAllThreads([plugin = plugin_](ThreadLocal::ThreadLocalObjectSharedPtr object) | ||
| -> ThreadLocal::ThreadLocalObjectSharedPtr { | ||
| object->asType<WasmHandle>().wasm()->startShutdown(plugin); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you merge main to pick up @jmarantz changes on this? We have the typed wrappers now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, although it doesn't look like the typed wrappers support storing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct. having nullptr as a possibility was not a requirement for any of the existing uses, unless you are not storing a value at all. Is that a problem?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you added a layer of indirection through a handle which seems like a waste. We could change runOnAllThreads send in a typed pointer allowing nullptr or absl::optional ref or add a couple of new variants of runOnAllThreads. @PiotrSikora I'm not sure what the use-case is though for allowing null. Wouldn't you want to always set() your tls container on startup? @mattklein123 WDYT?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like allowing storing nullptr in the slot should be OK? Should we just fix that?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah my preference would be for not having new variants. Maybe I'm missing something but can't we just make it so that shared_ptr that is stored can be null?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shared_ptr can be stored as null, but the call to asType() in the template wrapper does a dynamic cast and ASSERT. That's easy to fix but then we have to pass the thing as something that can be checked for null
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Yeah I would probably just do optional shared_ptr.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry optional reference.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #13883 prototypes that change. Would like to get some quick feedback on the direction prior to filling out comments and adding a unit test for new thin abstraction |
||
| return object; | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| std::string root_id_; | ||
| Common::Wasm::PluginSharedPtr plugin_; | ||
| ThreadLocal::SlotPtr tls_slot_; | ||
| AccessLog::FilterPtr filter_; | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.