wip: introduce safe callbacks and use in rewriting init manager#6245
wip: introduce safe callbacks and use in rewriting init manager#6245mergeconflict wants to merge 2 commits intoenvoyproxy:masterfrom mergeconflict:callback
Conversation
Signed-off-by: Dan Rosen <mergeconflict@google.com>
htuch
left a comment
There was a problem hiding this comment.
I like the fact that we're now moving to a safer-by-default model here. Some questions on the new model..
| * Explicit conversion to bool, to test whether the receiver contains a callback function. | ||
| * @return true if the corresponding Receiver contains a callback, false otherwise. | ||
| */ | ||
| explicit operator bool() const { return static_cast<bool>(fn_); } |
| @@ -0,0 +1,80 @@ | |||
| #include "init/callback.h" | |||
There was a problem hiding this comment.
This should probably be source/common/init, this isn't a top-level Envoy concern.
There was a problem hiding this comment.
I was wondering about that. The current state of master is sort of weird:
- The
Init::ManagerandInit::Targetinterfaces are defined ininclude/envoy/init - The
InitManagerImplclass is defined insource/serverwith a TODO comment, "Move InitManagerImpl into a new subdirectory in source/ called init/"
I agree it'd make sense to move the implementation into source/common/init. What would you think about also moving the interfaces down into include/envoy/common/init?
There was a problem hiding this comment.
source/common/init for implementation, include/envoy/init for interface; that's how Envoy canonical paths work.
| * Caller and Receiver are simple wrappers for their counterparts in Common::Callback with logging | ||
| * added to help debug initialization and destruction ordering issues that occasionally arise. | ||
| */ | ||
| class Caller : Logger::Loggable<Logger::Id::init> { |
There was a problem hiding this comment.
Wait, why is there a Caller/Receiver defined here as well as earlier on? :) Can't we just have one abstract version of this?
There was a problem hiding this comment.
The Caller/Receiver in common/callback are meant to be as bare-bones as possible, with no logging included. Going on the assumption that we want safer callbacks everywhere, not just in initialization, I didn't want to bake logging as a requirement.
There was a problem hiding this comment.
We definitely want to converge these implementations IMHO; even if we have to use macro magic or somethign to do it. They are pretty complicated to reason about, having only a single one is the way to go.
Signed-off-by: Dan Rosen <mergeconflict@google.com>
Introduce a new "safe" init manager, to replace the existing one that's prone to use-after-free issues (see e.g. #6116). Users of the existing init manager will be upgraded one-by-one in subsequent PRs if this design is approved. See also previous false starts in PRs #6136 and #6245. Risk Level: Low, no existing users of the existing init manager are changed in this PR. Testing: New unit tests added. Docs Changes: n/a Release Notes: n/a Signed-off-by: Dan Rosen <mergeconflict@google.com>
Introduce a new, "safe" callback mechanism (as in, it's safe to invoke the callback after the receiver has been destroyed), and use it as the basis for a new init manager. See #6116 and #6136 for background.
This is work in progress; I haven't updated use sites yet. Seeking feedback on the basic design.
Signed-off-by: Dan Rosen mergeconflict@google.com