init: replace old init manager with with new "safe" init manager#6360
init: replace old init manager with with new "safe" init manager#6360mattklein123 merged 6 commits intoenvoyproxy:masterfrom mergeconflict:safe_init_conversion
Conversation
Signed-off-by: Dan Rosen <mergeconflict@google.com>
|
/review @htuch @mattklein123 @lizan This is a mostly mechanical, uninteresting set of changes. I've deleted the old |
|
/retest |
|
🙀 failed invoking rebuild of |
mattklein123
left a comment
There was a problem hiding this comment.
Very cool stuff. A few comments from a quick skim, will defer to @htuch for full review. Very nice!
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Signed-off-by: Dan Rosen <mergeconflict@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks looks great to me. Great work! I will defer to @htuch who might want to take a pass.
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo Matt's comments and those here.
/wait
| makeSingleRequest(); | ||
| } | ||
|
|
||
| // Regression test for the use-after-free crash when a listener awaiting an RDS update is destroyed |
There was a problem hiding this comment.
Can you move this to another PR? I think it doesn't logically belong with this. Ideally, the changes in this PR are all mechanical.
There was a problem hiding this comment.
I could, and you're right, these are mostly mechanical changes ... but I want to try to push back a little. The fix for #6116 is in this PR, and I feel like it's a little weird for the regression test to land separately.
Signed-off-by: Dan Rosen <mergeconflict@google.com>
| Common::CallbackManager<> update_callback_manager_; | ||
|
|
||
| private: | ||
| void initialize(); |
There was a problem hiding this comment.
nit: newline between methods and members.
Description: Replace the old init manager with the new "safe" init manager and update all use sites.
Risk Level: Low
Testing: All tests pass, including a new integration test to cover the specific crash observed in #6116.
Docs Changes: n/a
Release Notes: n/a
Fixes #6116
Signed-off-by: Dan Rosen mergeconflict@google.com