server: fix server lifecycle notifier when callback list is empty#7103
server: fix server lifecycle notifier when callback list is empty#7103mattklein123 merged 1 commit intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Elisha Ziskind <eziskind@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
LGTM basically, but 2 nits:
- can we use thread-factory rather than going directly to std::thread
- I assume the old impl would fail with the new test. How would it fail? The assert on line old 619?
| ENVOY_LOG(info, "Notifying {} callback(s) with completion.", it2->second.size()); | ||
| for (const StageCallbackWithCompletion& callback : it2->second) { | ||
| callback(wrapped_cb); | ||
| callback([cb_guard] { (*cb_guard)(); }); |
There was a problem hiding this comment.
TIL: the () in lambdas is optional.
| // have finished their work. | ||
| std::shared_ptr<Event::PostCb> cb_guard(new Event::PostCb([] {}), | ||
| [this, completion_cb](Event::PostCb* cb) { | ||
| ASSERT(std::this_thread::get_id() == main_thread_id_); |
There was a problem hiding this comment.
I hadn't seen the original PR, but why are we not using the thread-factory abstraction here? We have easy access to it via server_->api..
There was a problem hiding this comment.
No special reason - I think I just copied a similar pattern used in TLS
| // Wrap completion_cb so that it only gets invoked when all callbacks for this stage | ||
| // have finished their work. | ||
| std::shared_ptr<Event::PostCb> cb_guard(new Event::PostCb([] {}), | ||
| [this, completion_cb](Event::PostCb* cb) { |
There was a problem hiding this comment.
TIL: you can specify a deleter with a shared-pointer. Very cool.
As mentioned, I was just copying the pattern used in TLS. Do you want me to switch to use the thread factory in this PR?
Yes - the old assert would fail with the new test. |
|
No, let's not grow the PR especially as there were already-existing uses. Those will all have to be cleaned up, I presume, at some point for portability. |
5 participants
Description: Now that lifecycle notification registrations can be canceled (with #6984) the list of registered callbacks could be empty, so need to remove an ASSERT. Also simplified the code to defer execution of the completion_cb until all registered callbacks have been notified - copied the approach used in #7011 which wraps the completion callback in a shared_ptr. This allows removing the special-case code where there are no registered callbacks.
Risk Level: low
Testing: unit tests
Signed-off-by: Elisha Ziskind eziskind@google.com