-
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
[dispatcher] Refactor how callbacks are scheduled in the event loop. #11663
[dispatcher] Refactor how callbacks are scheduled in the event loop. #11663
Conversation
…n in the event loop. Also migrate existing users of Timer::enableTimer(0ms) that require immediate execution of the callback to use this new interface instead. Signed-off-by: Antonio Vicente <[email protected]>
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, LGTM with small questions/comments. Thank you!
/wait
include/envoy/event/schedulable_cb.h
Outdated
/** | ||
* Schedule the callback so it runs in the current iteration of the event loop. | ||
*/ | ||
virtual void scheduleCallback() PURE; |
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.
Is your intention to eventually also handle the 0ms timers that should run in the next/future event loop also via this interface? Will you eventually take an enum here or have a different function for that? Should we add the enum or name this function with that in mind to avoid having to just change it in a follow up?
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.
Eventually I'ld like to get 0ms timers to execute in the next iteration by switching away from event_active as an activation method for 0ms timers. I expect that real timers won't depend on this class.
I do want to use this new interface for fd activations that should happen the next iteration. The relevant code change is staged at: antoniovicente@2194979
The question is if we would prefer scheduleCallback to take a schedule type as an argument or we rather have separate methods for immediate and delayed activations. Use of 2 methods would be less verbose since you wouldn't need to pass in the fully qualified nested enum name. How about something like scheduleCallbackCurrentIteration / scheduleCallbackNextIteration (or scheduleCurrentIteration / scheduleNextIteration)
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.
2 methods is fine with me. I don't feel strongly about it. Do you want to do the naming in this 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.
I'm leaning towards changing the name to scheduleCallbackCurrentIteration in this PR, and introducing scheduleCallbackNextIteration in the next PR along with uses and tests.
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.
SGTM.
/wait
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.
Signed-off-by: Antonio Vicente <[email protected]>
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.
Thanks for the review!
include/envoy/event/schedulable_cb.h
Outdated
/** | ||
* Schedule the callback so it runs in the current iteration of the event loop. | ||
*/ | ||
virtual void scheduleCallback() PURE; |
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.
Eventually I'ld like to get 0ms timers to execute in the next iteration by switching away from event_active as an activation method for 0ms timers. I expect that real timers won't depend on this class.
I do want to use this new interface for fd activations that should happen the next iteration. The relevant code change is staged at: antoniovicente@2194979
The question is if we would prefer scheduleCallback to take a schedule type as an argument or we rather have separate methods for immediate and delayed activations. Use of 2 methods would be less verbose since you wouldn't need to pass in the fully qualified nested enum name. How about something like scheduleCallbackCurrentIteration / scheduleCallbackNextIteration (or scheduleCurrentIteration / scheduleNextIteration)
Looks like needs a master merge also. /wait |
Signed-off-by: Antonio Vicente <[email protected]>
Signed-off-by: Antonio Vicente <[email protected]>
Signed-off-by: Antonio Vicente <[email protected]>
|
||
/** | ||
* Schedule the callback so it runs in the current iteration of the event loop. | ||
*/ |
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.
Are there any ordering guarantees that this API provides? If so it would be good to document them.
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.
Note that the semantics for scheduleCallbackCurrentIteration and scheduleCallbackNextIteration (which I will be introducing in the next PR) are very different. scheduleCallbackNextIteration will provide little to no ordering guarantees since the target implementation based on timers at 0ms provides no ordering guarantees (items go through an unordered priority queue)
Signed-off-by: Antonio Vicente <[email protected]>
LGTM. Can you check CI? /wait |
Looks like a pre-existing flake in //test/integration:protocol_integration_test that alyssawilk seems to be looking at after I mentioned it in the envoy-ci slack channel. The CI run before the most recent change to comments was successful. |
Signed-off-by: Antonio Vicente <[email protected]>
/azp run envoy-presubmit |
Commenter does not have sufficient privileges for PR 11663 in repo envoyproxy/envoy |
Signed-off-by: Antonio Vicente <[email protected]>
…nvoyproxy#11663) Introduce a separate interface for to schedule callbacks for execution in the event loop. Also migrate existing users of Timer::enableTimer(0ms) that require immediate execution of the callback to use this new interface instead. Signed-off-by: Antonio Vicente <[email protected]>
…nvoyproxy#11663) Introduce a separate interface for to schedule callbacks for execution in the event loop. Also migrate existing users of Timer::enableTimer(0ms) that require immediate execution of the callback to use this new interface instead. Signed-off-by: Antonio Vicente <[email protected]> Signed-off-by: yashwant121 <[email protected]>
…e event loop (#11823) Processing 0-delay timers in the same loop they are generated can result in long timer callback chains which could starve other operations in the event loop or even result in infinite processing loops. Cases that required same-iteration scheduling behavior for 0-delay timers were refactored to use SchedulableCallback::scheduleCallbackCurrentIteration in #11663, so behavior changes due to this change should be relatively minor. Signed-off-by: Antonio Vicente <[email protected]>
Commit Message: Introduce a separate interface for to schedule callbacks for execution in the event loop. Also migrate existing users of Timer::enableTimer(0ms) that require immediate execution of the callback to use this new interface instead.
Additional Description: This is a functional no-op change in preparation for changes to the behavior of fd activations and 0ms timers. Some event loop callbacks scheduled via 0ms Timers do need to execute in the same event loop iteration they are scheduled in order to provide optimal behavior.
Risk Level: n/a - functional no-op
Testing: unit
Docs Changes: n/a
Release Notes: n/a