Skip to content

feat: all executors now create daemon threads to reduce shutdown time #1715

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: all executors now create daemon threads to reduce shutdown time #1715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/EventProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
public abstract class EventProvider implements FeatureProvider {
private EventProviderListener eventProviderListener;
private final ExecutorService emitterExecutor =
Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-emitter-thread"));
Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-emitter-thread", true));
Comment on lines 25 to +26
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Changing the threads to daemon threads introduces a risk. Daemon threads can be abruptly terminated by the JVM on shutdown, which does not guarantee the execution of finally blocks.

In the task submitted to emitterExecutor (lines 93-104), there is a finally block that calls awaitable.wakeup() and a try-with-resources statement that releases a read lock. If this thread is terminated abruptly, the Awaitable might never be woken up, and the lock might not be released.

While the shutdown() method attempts a graceful shutdown, using daemon threads can hide lifecycle management issues where shutdown() is not called. Given the critical operations within the finally block, it might be safer to use non-daemon threads to ensure cleanup logic is always executed, forcing the SDK user to manage the provider lifecycle correctly.

Copy link
Contributor

@chrfwow chrfwow Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we care about a lock not getting released when the JVM is terminated


void setEventProviderListener(EventProviderListener eventProviderListener) {
this.eventProviderListener = eventProviderListener;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/EventSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class EventSupport {
private final Map<String, HandlerStore> handlerStores = new ConcurrentHashMap<>();
private final HandlerStore globalHandlerStore = new HandlerStore();
private final ExecutorService taskExecutor =
Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-handler-thread"));
Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-handler-thread", true));
Comment on lines 30 to +31
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to my comment on EventProvider.java, changing the threads for taskExecutor to daemon threads has risks. The tasks run by this executor are user-provided event handlers (handler.accept(eventDetails) on line 132).

If these handlers perform operations that require cleanup in finally blocks (e.g., closing files, releasing locks, committing transactions), using daemon threads means this cleanup is not guaranteed to run if the JVM shuts down. This could lead to resource leaks or data corruption.

It would be safer to use non-daemon threads to ensure that handlers can complete their work, or at least their cleanup routines, during a graceful shutdown. This would enforce correct lifecycle management by the user of the SDK.


/**
* Run all the event handlers associated with this domain.
Expand Down
Loading