xds: keep a reference to old config in the filter config provider#12479
xds: keep a reference to old config in the filter config provider#12479mattklein123 merged 6 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
cc @jplevyak |
|
I'll add a test if we can agree on the approach. I'd need to mock a filter with a slot to be truthful to the original issue |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
I think this is a fine approach. The history here around why TLS gets deleted on the main thread is around simplicity. It's come up a few times to allow this to happen from any thread. I'm not sure if you want to tackle that or not. Thank you for fixing either way!
/wait
| } | ||
| return previous; | ||
| }, | ||
| [old_config]() {}); |
There was a problem hiding this comment.
Can you add more comments in the final change?
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
PTAL. Tested with --runs_per_test =1000 and found that the previous attempt didn't work (it was capturing |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
Fixes #12315 |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM with small comments.
/wait
|
|
||
| FilterConfigSubscriptionSharedPtr subscription_; | ||
| const std::set<std::string> require_type_urls_; | ||
| absl::optional<Envoy::Http::FilterFactoryCb> config_{absl::nullopt}; |
There was a problem hiding this comment.
nit: I don't think this needs to be an optional? It can just be a null lambda? Also can we call it current_config_ or something like that with a comment about what this member is for?
There was a problem hiding this comment.
I think it's good for consistency with the rest. Lambda pointer is rather confusing and doesn't own the underlying values which is the intent. I'll change the name and comment.
| auto filter_config = std::make_shared<SetResponseCodeFilter>(proto_config.prefix(), | ||
| proto_config.code(), context); |
There was a problem hiding this comment.
nit: can we make this more realistic. The filter should be allocated inside the lambda below, with the TLS slot allocated outside. This could be done by just allocating the slot outside and passing it, or have a config object and passing that to the filter as is done elsewhere.
There was a problem hiding this comment.
I was just copying what wasm filter did. I guess it's not idiomatic.
* merge fix Signed-off-by: Kuat Yessenov <kuat@google.com> * format fix Signed-off-by: Kuat Yessenov <kuat@google.com>
…voyproxy#12479) Some filter factories allocate TLS slots shared via shared pointers. On a filter config update, the last filter factory reference happens to be deleted on a worker thread, which causes a runtime failure since TLS slots must be deleted on the main thread. The solution is to prolong the life of the filter factory using main thread completion callback. Signed-off-by: Kuat Yessenov <kuat@google.com>
…voyproxy#12479) Some filter factories allocate TLS slots shared via shared pointers. On a filter config update, the last filter factory reference happens to be deleted on a worker thread, which causes a runtime failure since TLS slots must be deleted on the main thread. The solution is to prolong the life of the filter factory using main thread completion callback. Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: chaoqinli <chaoqinli@google.com>
3 participants
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: Some filter factories allocate TLS slots shared via shared pointers. On a filter config update, the last filter factory reference happens to be deleted on a worker thread, which causes a runtime failure since TLS slots must be deleted on the main thread. The solution is to prolong the life of the filter factory using main thread completion callback.
Fixes envoyproxy/envoy-wasm#601
Additional Description:
Risk Level: low
Testing: manual
Docs Changes:
Release Notes: