-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
grid: changing to defered deletion #34832
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
This one in not a hurry as I'm still working on tests for the final PR :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think the getOrCreate is a nice cleanup.
source/common/http/conn_pool_grid.h
Outdated
// TODO(alyssawilk) replace this now we have explicit pools. | ||
virtual ConnectionPool::Instance* createNextPool(); | ||
// Creates the requested pool | ||
virtual ConnectionPool::Instance* getOrCreateHttp3Pool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tests, it seems that we really only want to override the "create" part of this method (not the "get" part or the "setupPool" parts, etc). So how 'bout we cook up createHttp3Pool()
and createHttp2Pool()
and make them virtual. Then they can be called from the non-virutal getOrCreate()
. This would simplify the mocking. (Alternatively we could pass two absl::AnyInvokable
s in the constructor and call them without using any virtual functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
source/common/http/conn_pool_grid.cc
Outdated
@@ -324,8 +321,7 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder& | |||
} | |||
} else { | |||
// Make sure the HTTP/2 pool is created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably delete this comment now since:
pool = getOrCreateHttp2Pool()
is super readable. Woo hoo!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -356,8 +351,7 @@ void ConnectivityGrid::addIdleCallback(IdleCb cb) { | |||
|
|||
void ConnectivityGrid::drainConnections(Envoy::ConnectionPool::DrainBehavior drain_behavior) { | |||
if (drain_behavior == Envoy::ConnectionPool::DrainBehavior::DrainAndDelete) { | |||
// Note that no new pools can be created from this point on | |||
// as createNextPool fast-fails if `draining_` is true. | |||
// Note that no new pools should be created from this point on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "should be" => "will be" since this invariant is enforced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is, there's just an assert?
we used to handle null pools, now we'll create on drain.
source/common/http/conn_pool_grid.h
Outdated
virtual ConnectionPool::Instance* createNextPool(); | ||
// Creates the requested pool | ||
virtual ConnectionPool::Instance* getOrCreateHttp3Pool(); | ||
virtual ConnectionPool::Instance* getOrCreateHttp2Pool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC, can these methods return null? If not, maybe they should return refs? (But I kinda think they can return null?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they shouldn't return null but the callers want pointers so I didn't think changing the types made sense?
source/common/http/conn_pool_grid.h
Outdated
// Creates the next pool in the priority list, or nullptr if all pools have been created. | ||
// TODO(alyssawilk) replace this now we have explicit pools. | ||
virtual ConnectionPool::Instance* createNextPool(); | ||
// Creates the requested pool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: How about "Returns the specified pool, which will be created if necessary"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
public: | ||
WrapperCallbacks(ConnectivityGrid& grid, Http::ResponseDecoder& decoder, | ||
ConnectionPool::Callbacks& callbacks, const Instance::StreamOptions& options); | ||
|
||
bool hasNotifiedCaller() { return inner_callbacks_ == nullptr; } | ||
void deleteIsPending() override { next_attempt_timer_.reset(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the relationship between the next attempt timer and delete pending?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented inline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see. I missed that this was a virtual method as part of Event::DeferredDeletable
. Mind adding:
``// Event::DeferredDeletable:`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
source/common/http/conn_pool_grid.cc
Outdated
return ret; | ||
|
||
// Return a handle if the caller hasn't yet been notified of success/failure. | ||
// Note there may still be connection attempts, if we'e waiting on a slow H3 connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "if we'e waiting on" => "if the TCP attempt is delayed and the H3 attempt is slow" (or somesuch)
go/avoid-we
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrased
return nullptr; | ||
ConnectionPool::Instance* getOrCreateHttp3Pool() override { | ||
if (!http3_pool_) { | ||
http3_pool_.reset(createMockPool("http3")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: http3_pool_ = createMockPool("http3")
(ditto below) or alternatively if we go with making createFooPool()
methods, then this is even simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Two minor comments.
static ConnectionPool::Instance* forceGetOrCreateHttp3Pool(ConnectivityGrid& grid) { | ||
return grid.getOrCreateHttp3Pool(); | ||
} | ||
static ConnectionPool::Instance* forceGetOrCreateHttp2Pool(ConnectivityGrid& grid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since we're now doing "using ConnectivityGrid::getOrCreateHttp2Pool" do we need this helper function, or can we call grid.getOrCreateHttp2Pool() directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default the test grid creates mock pools. This allows forcing a real grid's real pools. commented inline
public: | ||
WrapperCallbacks(ConnectivityGrid& grid, Http::ResponseDecoder& decoder, | ||
ConnectionPool::Callbacks& callbacks, const Instance::StreamOptions& options); | ||
|
||
bool hasNotifiedCaller() { return inner_callbacks_ == nullptr; } | ||
void deleteIsPending() override { next_attempt_timer_.reset(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see. I missed that this was a virtual method as part of Event::DeferredDeletable
. Mind adding:
``// Event::DeferredDeletable:`
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion. currently we infer immediate response / async newStream from return codes. This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete. Also doing a follow-up from the prior PR and creating connection pools explicitly. Risk Level: high (object lifetime changes) Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: antoniovleonti <leonti@google.com>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion. currently we infer immediate response / async newStream from return codes. This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete. Also doing a follow-up from the prior PR and creating connection pools explicitly. Risk Level: high (object lifetime changes) Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion. currently we infer immediate response / async newStream from return codes. This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete. Also doing a follow-up from the prior PR and creating connection pools explicitly. Risk Level: high (object lifetime changes) Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion. currently we infer immediate response / async newStream from return codes. This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete. Also doing a follow-up from the prior PR and creating connection pools explicitly. Risk Level: high (object lifetime changes) Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion. currently we infer immediate response / async newStream from return codes. This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete. Also doing a follow-up from the prior PR and creating connection pools explicitly. Risk Level: high (object lifetime changes) Testing: existing tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
changing the wrapper callbacks and async callbacks in the grid to do deferred deletion.
currently we infer immediate response / async newStream from return codes.
This gets more complicated when we add in happy eyeballs, so instead we want to determine if we should return a cancelable handle based on if the caller has been informed of success or failure. This requires the objects to not be deleted under the stack of newStream hence the migration to defered delete.
Also doing a follow-up from the prior PR and creating connection pools explicitly.
Risk Level: high (object lifetime changes)
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a