Skip to content

Conversation

@nicklasl
Copy link
Member

@nicklasl nicklasl commented Nov 26, 2025

Summary

Improves the shutdown behaviour of the ProviderRepository to ensure safe termination of the task executor. The previous implementation called shutdown() on the executor but did not wait for termination, potentially leading to abrupt shutdown, aborted provider shutdown sequences or hanging threads.

Changes

  • Added graceful shutdown handling in ProviderRepository.shutdown() with a 3-second timeout
  • Added shutdown state tracking to prevent race conditions
  • Handles RejectedExecutionException when tasks are submitted to a shut down executor

Implementation Details

The shutdown process now:

  1. Calls shutdown() on the task executor to prevent new tasks
  2. Waits up to 3 seconds for running tasks to complete via awaitTermination()
  3. If tasks don't complete within the timeout, calls shutdownNow() to force termination
  4. Properly handles InterruptedException by calling shutdownNow() and restoring the interrupt flag
  5. Prevents new providers from being registered during shutdown via AtomicBoolean flag
  6. Handles RejectedExecutionException by running provider shutdown synchronously when executor is closed

Test 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.

@nicklasl nicklasl force-pushed the fix-shutdown-behaviour branch from bb09ece to 08e5fc7 Compare November 26, 2025 07:59
@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.60%. Comparing base (bf86db5) to head (6ac560c).

Files with missing lines Patch % Lines
...n/java/dev/openfeature/sdk/ProviderRepository.java 95.45% 1 Missing ⚠️
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     
Flag Coverage Δ
unittests 93.60% <95.45%> (+1.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nicklasl nicklasl force-pushed the fix-shutdown-behaviour branch from 7271a12 to dc340c1 Compare November 27, 2025 12:35
@nicklasl
Copy link
Member Author

I added a commit to additionally prevent race conditions during shutdown:

  • Added AtomicBoolean isShuttingDown flag to track repository shutdown state
  • setProvider() now throws IllegalStateException if called during shutdown
  • Handles RejectedExecutionException when shutdown tasks try to submit to closed executor
  • Ensures idempotent shutdown (multiple concurrent calls are safe)

@sonarqubecloud
Copy link

@nicklasl nicklasl marked this pull request as ready for review November 27, 2025 12:42
@nicklasl nicklasl requested review from a team as code owners November 27, 2025 12:42
Copy link
Contributor

@chrfwow chrfwow left a 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();
Copy link
Contributor

@chrfwow chrfwow Nov 27, 2025

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

Comment on lines +462 to +464
Thread t1 = new Thread(() -> providerRepository.shutdown());
Thread t2 = new Thread(() -> providerRepository.shutdown());
Thread t3 = new Thread(() -> providerRepository.shutdown());
Copy link
Contributor

@chrfwow chrfwow Nov 27, 2025

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

Copy link
Member Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants