python: keep stream and engine alive for callbacks#1379
Conversation
crockeo
force-pushed
the
ch/callback-lifetimes
branch
from
0a00df3 to
53a46a4
Compare
Signed-off-by: Cerek Hillen <chillen@lyft.com>
crockeo
force-pushed
the
ch/callback-lifetimes
branch
from
44263e7 to
3b7f3e7
Compare
…lback-lifetimes Signed-off-by: Cerek Hillen <chillen@lyft.com>
|
Marking as ready for review because I want feedback on this strategy. I'm kind of fumbling around in the dark, so I'm sure there's a better way. |
| .log = nullptr, | ||
| .release = envoy_noop_release, | ||
| .context = nullptr, | ||
| }; |
There was a problem hiding this comment.
Would need to verify, but by convention, it should be safe to simply calloc this (or any other envoy mobile struct) if you're not going to use it.
…lback-lifetimes Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
…lback-lifetimes Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
…lback-lifetimes Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
…lback-lifetimes Signed-off-by: Cerek Hillen <chillen@lyft.com>
Signed-off-by: Cerek Hillen <chillen@lyft.com>
library/cc/engine.cc
Outdated
| // which can't be provided from inside of the constructor | ||
| // because of how std::enable_shared_from_this works | ||
| StreamClientSharedPtr Engine::streamClient() { | ||
| if (!this->stream_client_) { |
There was a problem hiding this comment.
This looks like it's not going to be thread-safe. If we need to use lazy clients, we should maybe just create a new one every time someone asks for one. It's not an especially heavyweight object anyways.
library/cc/engine.cc
Outdated
| // because of how std::enable_shared_from_this works | ||
| StreamClientSharedPtr Engine::streamClient() { | ||
| if (!this->stream_client_) { | ||
| this->stream_client_ = std::make_shared<StreamClient>(this->weak_from_this()); |
There was a problem hiding this comment.
I think StreamClient should probably take a shared reference to the Engine rather than a weak one. We made stats weak so that all the stats objects that might be distributed around a program wouldn't all be retaining the engine... but then again, it's up for debate whether they should be, as well.
There was a problem hiding this comment.
(Also FWIW, it does effectively take a shared ref in the other platforms.)
There was a problem hiding this comment.
This is a weak ptr to avoid a cycle (Engine -> StreamClient -> Engine), but I don't think this cycle needs to exist anymore. It's just a byproduct of the original design of this PR where Streams kept Engines alive.
I can address this comment along with the earlier comment at the same time, I think, by just removing the EngineSharedPtr references throughout StreamClient and StreamPrototype and just move them back to envoy_engine_t
2 participants
Description: Modifies the ownership model of objects so that callbacks keep their relevant parent alive (
EngineCallbacks->Engine;StreamCallbacks->Stream) instead of the other way around.This PR assumes that one of
StreamCallbacksterminal callbacks will be called exactly once (on_complete,on_error,on_cancel) and thaton_exitwill be called exactly once. If these are called more than once there will be a double free, if they are not called once we'll leak memory. This is obviously not good 🙂Risk Level: Low
Testing:
bazel test //test/python/integration:test_lifetimesDocs Changes: N/A
Release Notes: N/A