Improve Thread Management in OpenFeature: Use Named Daemon Threads

[aepfli]

When using OpenFeature, I noticed that the service takes longer than expected to shut down under certain conditions, such as when the HTTP port is already in use. Upon debugging, I found that this behavior is caused by OpenFeature's use of a ThreadPoolExecutor that creates non-daemon threads.

Non-daemon threads prevent the JVM from shutting down immediately, which can lead to delays and potentially block application termination. Additionally, the threads created by OpenFeature are named with the default pattern (e.g., pool-6-thread-1), making it difficult to identify their source during debugging or thread dumps.

Proposed Improvements

1. Switch to Daemon Threads:

   • Update the ThreadPoolExecutor to create daemon threads. Daemon threads allow the JVM to shut down immediately when the application exits, improving shutdown behavior.

2. Use Descriptive Thread Names:

   • Update the thread factory to assign descriptive names to threads, such as including the prefix openfeature. This makes it easier to identify threads in thread dumps and logs.

Reasoning

• Faster Shutdowns: Non-daemon threads can cause the application to hang during shutdown until all threads are terminated. Switching to daemon threads resolves this issue.
• Debugging and Monitoring: Default thread names like pool-6-thread-1 provide no context about their origin, making debugging thread-related issues more challenging. Using descriptive names improves observability and troubleshooting.
• Consistency: Aligning with best practices for thread management ensures better integration with user applications and reduces surprises during runtime.

Steps to Reproduce

1. Start an application using OpenFeature.
2. Simulate an error scenario, such as attempting to bind to an already-used HTTP port.
3. Observe the delay in application shutdown.
4. Inspect the thread dump and note the presence of non-daemon threads with default names (e.g., pool-6-thread-1).

Suggested Fix

• Update the ThreadPoolExecutor initialization to use a custom ThreadFactory that:
  • Creates daemon threads (thread.setDaemon(true)).
  • Assigns descriptive names to threads (e.g., openfeature-thread-{id}).

References

• Java Thread Daemon Documentation
• Example of setting a custom ThreadFactory:

ThreadFactory threadFactory = new ThreadFactoryBuilder()
    .setNameFormat("openfeature-thread-%d")
    .setDaemon(true)
    .build();
ExecutorService executor = Executors.newFixedThreadPool(4, threadFactory);

Additional Context

The issue was observed in the following version of OpenFeature: [Add version here].
If this behavior is intentional or there are constraints around making threads daemon, it would be helpful to document this in the project.

Impact

• Applications using OpenFeature may experience delayed shutdowns, especially during error scenarios.
• Debugging and monitoring thread-related issues are more difficult due to non-descriptive thread names.

[aepfli]

@chrfwow i am really curious about your opinion regarding this.

[chrfwow]

We can certainly give our thread pool threads descriptive names. This could really help with debugging. As for deamon/non deamon, we would have to make sure that our shutdown logic, and also that in the providers, can be terminated at any time, which might be hard to guarantee. I don't know if that is currently the case

[jarebudev]

i can pick this one up

[chrfwow]

@jarebudev I assigned this ticket to you. Reach out if you need anything!

[jarebudev]

Just to update - I've done most of this, however I am out next week so will finish it off when I'm back.

[shaoqin2]

We as a library provider uses flagd internally, we have no reliable way of requiring clients to invoke a shutdown method. Our threads are all daemons.

Flagd side not being a daemon thread hangs our client if used in a CLI fashion by our clients. Definitely supportive of making them daemon threads.

To @chrfwow earlier concern, since the entire JVM is shutting down, do we need to worry about flagd internal thread state at that point?

[chrfwow]

@shaoqin2 yes, it should be fine, the upstream services we connect to have to be able to handle a loss of connection anyway

[shaoqin2]

@jarebudev wondering if you are going to come back to this work - I'm happy to put out a change for daemon thread if that's ok. We are kinda waiting for that change to enable flagd for our client side library usage.

[jarebudev]

@shaoqin2 - yeh - have done the naming just need to now change over to creating daemon threads. Will pick this up now