-
Notifications
You must be signed in to change notification settings - Fork 52
fix: allow for providers to safely shutdown #1744
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
base: main
Are you sure you want to change the base?
Conversation
bb09ece to
08e5fc7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1744 +/- ##
============================================
+ Coverage 92.51% 93.60% +1.08%
- Complexity 593 600 +7
============================================
Files 53 53
Lines 1403 1422 +19
Branches 150 151 +1
============================================
+ Hits 1298 1331 +33
+ Misses 64 53 -11
+ Partials 41 38 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Nicklas Lundin <nicklasl@spotify.com>
Signed-off-by: Nicklas Lundin <nicklasl@spotify.com>
7271a12 to
dc340c1
Compare
|
I added a commit to additionally prevent race conditions during shutdown:
|
Signed-off-by: Nicklas Lundin <nicklasl@spotify.com>
|
chrfwow
left a comment
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.
The implementation looks nice, but the tests may have some race conditions
| setFeatureProvider(provider); | ||
|
|
||
| Thread shutdownThread = new Thread(() -> { | ||
| providerRepository.shutdown(); |
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.
How long does the Mocked Provider take to shutdown? Are we sure we interrupt during this time?
Maybe this is a case for a VMLens test
| Thread t1 = new Thread(() -> providerRepository.shutdown()); | ||
| Thread t2 = new Thread(() -> providerRepository.shutdown()); | ||
| Thread t3 = new Thread(() -> providerRepository.shutdown()); |
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.
We cannot be sure that any of these threads execute in parallel. This should be a VMLens test
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, I'll look into making them VMLens tests instead, just need to find some time :)



Summary
Improves the shutdown behaviour of the
ProviderRepositoryto ensure safe termination of the task executor. The previous implementation calledshutdown()on the executor but did not wait for termination, potentially leading to abrupt shutdown, aborted provider shutdown sequences or hanging threads.Changes
ProviderRepository.shutdown()with a 3-second timeoutRejectedExecutionExceptionwhen tasks are submitted to a shut down executorImplementation Details
The shutdown process now:
shutdown()on the task executor to prevent new tasksawaitTermination()shutdownNow()to force terminationInterruptedExceptionby callingshutdownNow()and restoring the interrupt flagAtomicBooleanflagRejectedExecutionExceptionby running provider shutdown synchronously when executor is closedTest Coverage
Added comprehensive test coverage for shutdown behavior:
[x] Successful shutdown when executor terminates within timeout
[x] Forced shutdown when executor does not terminate within timeout
[x] Graceful handling of interruption during shutdown
[x] Protection against indefinite hangs during shutdown
[x] Concurrent shutdown calls (idempotent behavior)
[x] Shutdown during provider initialization
[x] Provider replacement during shutdown
[x] Prevention of new provider registration after shutdown starts
Related Issues
#1745
This change aligns with the broader effort to improve shutdown behaviour across the SDK, similar to PR #873 which addresses the same issue on Eventing.