You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR unifies the various shutdown synchronization types across multiple crates into a single pair of types in saluki-common.
Prior to this PR, we had five distinct shutdown types/pairs of types:
DynamicShutdownCoordinator/DynamicShutdownHandle: allowed for registering shutdown handles dynamically
ComponentShutdownCoordinator/ComponentShutdownHandle: basically DynamicShutdownCoordinator with a different name (my brain wasn't firing on all cylinders when I wrote this, clearly)
ShutdownHandle/ProcessShutdown: a paired shutdown trigger (like a oneshot sender/receiver pair) specifically for use in saluki_core::runtime
ShutdownTrigger/ShutdownHandle: a paired shutdown trigger added to saluki-common for general use where needed
ShutdownHandle, exposed for use with HttpServer to trigger shutdown
Conceptually, these are all very similar:
1 and 2 are basically identical: support registering multiple shutdown handles, then triggering them all at once... potentially waiting for them to complete their shutdown
3 and 4 are also basically identical, and they're a subset of 1 and 2: a trigger and a handle, except only one handle is ever registered
5 is just one half of the pair in 3 and 4
So I decided to unify things. In this PR, I've introduced a new pair of types for driving the sender/receiver sides of the equation: ShutdownCoordinator and ShutdownHandle. All instances of the previous five shutdown types have been replaced with these new types, fully unifying how we handle shutdown triggering/propagation. In no particular order:
ShutdownCoordinator supports multiple handles being registered
ShutdownCoordinator does a non-blocking shutdown trigger on drop, ensuring shutdown isn't missed when we hit errors/panics and don't reach the happy path shutdown call
ShutdownCoordinator supports both a blocking and non-blocking shutdown call: the blocking call (insofar as it waits, not that it's literally synchronous code) will wait until all shutdown handles have been dropped before returning
ShutdownHandle supports both synchronous and asynchronous code by exposing a method for synchronous code to check if shutdown has been triggered, while also being usable as a Future that resolves when shutdown is triggered
ShutdownHandle also exhibits fused/latching behavior in its Future implementation, easing potential bugs arising from misuse (re-polling resolved futures is generally bad)
ShutdownHandle also exposes a noop builder to allow creating handles that do nothing such that callers can pass a ShutdownHandle value to satisfy the required arguments for a method/type
Change Type
Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance
How did you test this PR?
Existing suite of tests -- unit, integration, correctness -- and a whole slew of new unit tests for the shutdown types.
Run ID:241134ff-25d4-4242-b392-b505987a1218 Baseline:559d98c2 · Comparison:b5a4d0d9 · diff
Optimization Goals: ✅ No significant changes detected
Fine details of change detection per experiment (35)
Experiments configured erratic: true are tagged (ignored) and skipped when determining which experiments regressed or improved. Experiments which are detected as erratic at runtime are tagged (erratic) to flag that the run's sample dispersion was high, but their regression / improvement signal still counts.
A change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression (is_regression: true). Improvements use the matching criteria for the improving direction. Experiments configured erratic: true (tagged (ignored)) are skipped outright; experiments detected as erratic at runtime (tagged (erratic)) still count, since that flag describes sample dispersion rather than directional certainty. The Δ mean % cell is colored accordingly: 🟢 = improvement, 🔴 = regression, ⚪ = neutral. Reduction in CPU or memory is an improvement; reduction in ingress throughput is a regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR unifies the various shutdown synchronization types across multiple crates into a single pair of types in
saluki-common.Prior to this PR, we had five distinct shutdown types/pairs of types:
DynamicShutdownCoordinator/DynamicShutdownHandle: allowed for registering shutdown handles dynamicallyComponentShutdownCoordinator/ComponentShutdownHandle: basicallyDynamicShutdownCoordinatorwith a different name (my brain wasn't firing on all cylinders when I wrote this, clearly)ShutdownHandle/ProcessShutdown: a paired shutdown trigger (like a oneshot sender/receiver pair) specifically for use insaluki_core::runtimeShutdownTrigger/ShutdownHandle: a paired shutdown trigger added tosaluki-commonfor general use where neededShutdownHandle, exposed for use withHttpServerto trigger shutdownConceptually, these are all very similar:
So I decided to unify things. In this PR, I've introduced a new pair of types for driving the sender/receiver sides of the equation:
ShutdownCoordinatorandShutdownHandle. All instances of the previous five shutdown types have been replaced with these new types, fully unifying how we handle shutdown triggering/propagation. In no particular order:ShutdownCoordinatorsupports multiple handles being registeredShutdownCoordinatordoes a non-blocking shutdown trigger on drop, ensuring shutdown isn't missed when we hit errors/panics and don't reach the happy path shutdown callShutdownCoordinatorsupports both a blocking and non-blocking shutdown call: the blocking call (insofar as it waits, not that it's literally synchronous code) will wait until all shutdown handles have been dropped before returningShutdownHandlesupports both synchronous and asynchronous code by exposing a method for synchronous code to check if shutdown has been triggered, while also being usable as aFuturethat resolves when shutdown is triggeredShutdownHandlealso exhibits fused/latching behavior in itsFutureimplementation, easing potential bugs arising from misuse (re-polling resolved futures is generally bad)ShutdownHandlealso exposes anoopbuilder to allow creating handles that do nothing such that callers can pass aShutdownHandlevalue to satisfy the required arguments for a method/typeChange Type
How did you test this PR?
Existing suite of tests -- unit, integration, correctness -- and a whole slew of new unit tests for the shutdown types.
References
DADP-2