core: queue API calls while underlying event dispatcher is not ready#428
core: queue API calls while underlying event dispatcher is not ready#428
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
|
This PR requires https://github.com/envoyproxy/envoy/pulls to land before it. |
library/common/http/dispatcher.cc
Outdated
|
|
||
| void Dispatcher::post(Event::PostCb callback) { | ||
| // If the event_dispatcher is present, dispatch the functor directly to it. | ||
| auto event_dispatcher = event_dispatcher_.load(); |
There was a problem hiding this comment.
Reading the code we've written through again, I think we could benefit down the road from encapsulating this logic in an EventQueue class. (I think we could also simplify the synchronized state handling as well by doing so.)
There was a problem hiding this comment.
EventQueue/CallbackQueue/InitQueue
mattklein123
left a comment
There was a problem hiding this comment.
Generally LGTM at a high level, though I would recommend simplifying the concurrency logic for now.
Signed-off-by: Mike Schore <mike.schore@gmail.com>
|
So I started to factor out the Ultimately, I'd still much rather work towards a solution where we get an earlier handle on the event_dispatcher_ which will be safer and simpler. |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
library/common/http/dispatcher.cc
Outdated
|
|
||
| void Dispatcher::ready(Event::Dispatcher& event_dispatcher, | ||
| Upstream::ClusterManager& cluster_manager) { | ||
| { |
There was a problem hiding this comment.
nit: there is no point in having these extra braces as you hold the lock for the length of the function. Same below.
library/common/http/dispatcher.cc
Outdated
| Upstream::ClusterManager& cluster_manager) | ||
| : event_dispatcher_(event_dispatcher), cluster_manager_(cluster_manager) {} | ||
| Dispatcher::Dispatcher() { | ||
| event_dispatcher_.store(nullptr); |
There was a problem hiding this comment.
I think you have a compile issue here.
Signed-off-by: Mike Schore <mike.schore@gmail.com>
library/common/http/dispatcher.h
Outdated
| class Dispatcher : public Logger::Loggable<Logger::Id::http> { | ||
| public: | ||
| Dispatcher(Event::Dispatcher& event_dispatcher, Upstream::ClusterManager& cluster_manager); | ||
| Dispatcher() {} |
Signed-off-by: Jose Nino <jnino@lyft.com>
|
tested locally against the unit test suite, and with all 4 demo apps. |
As of #428, we now queue API calls while Envoy starts up, so the 1s delay is no longer required. Removing the outdated docstrings. Signed-off-by: Michael Rebello <me@michaelrebello.com>
As of #428, we now queue API calls while Envoy starts up, so the 1s delay is no longer required. Removing the outdated docstrings. Signed-off-by: Michael Rebello <me@michaelrebello.com>
As of envoyproxy/envoy-mobile#428, we now queue API calls while Envoy starts up, so the 1s delay is no longer required. Removing the outdated docstrings. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
As of envoyproxy/envoy-mobile#428, we now queue API calls while Envoy starts up, so the 1s delay is no longer required. Removing the outdated docstrings. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: this PR adds a drainable queue to the http dispatcher that allows it to queue API calls while the underlying event dispatcher is not ready. It hooks into envoy's lifecycle notifier to flush the queue once envoy fires the post init lifecycle event.
Risk Level: med - changes startup behavior of the engine.
Testing: with example app and lyft alpha app. Will add unit tests in subsequent PR (#449)
Fixes #285
Co-authored-by: Mike Schore mike.schore@gmail.com
Signed-off-by: Jose Nino jnino@lyft.com