fix: Move event emitting off the main thread to avoid deadlocks #1314
Conversation
sideshowcoder
changed the title
sideshowcoder
force-pushed
the
sideshowcoder/issue/1299
branch
2 times, most recently
from
04e35fa to
20ba91c
Compare
@chrfwow has been contributing significantly to our Java implementations: - finally hook change in SDK: open-feature/java-sdk#1246 - flag metadata support in the flagd java provider: open-feature/java-sdk-contrib#1122 And most recently identified a deadlock in the Java SDK, opened an issue describing it, and has been helping to guide the fix by a new contributor: open-feature/java-sdk#1314 I'm nominating them for Java approver. @chrfwow please approve or 👍 this PR for signal your interest. Signed-off-by: Todd Baert <[email protected]>
sideshowcoder
force-pushed
the
sideshowcoder/issue/1299
branch
from
b9f3afe to
8c36616
Compare
|
This all makes sense to me. I see 2 test failures which I'm not really surprised about. It's possible that there's actually bugs here, but more likely it's inter-test interference or test-only race conditions revealed by this change. You can try running the failing tests individually and seeing if they work as expected. |
| public abstract class EventProvider implements FeatureProvider { | ||
| private EventProviderListener eventProviderListener; | ||
| private final ExecutorService emitterExecutor = Executors.newCachedThreadPool(runnable -> { | ||
| final Thread thread = new Thread(runnable); | ||
| thread.setDaemon(true); |
There was a problem hiding this comment.
daemon can have it stopped before emitting the remaining events and lose data.
The shutdown() method which is later called can gracefully shut it down with a timeout.
| public abstract class EventProvider implements FeatureProvider { | ||
| private EventProviderListener eventProviderListener; | ||
| private final ExecutorService emitterExecutor = Executors.newCachedThreadPool(runnable -> { |
There was a problem hiding this comment.
I think this can be simplified now that the thread is no longer modified
|
I am looking at the test failures locally but the seem to be passing, when run on their own, I continue to check what makes the CI run fail, I think there is some interaction going on. Just wanted to quickly update that I'm looking at this 👍 |
When stacking event emitting inside an EventProvider, when using sychronization the EventProvider can deadlock, to avoid this move the event emitting of the main thread. Signed-off-by: Philipp Fehre <[email protected]>
sideshowcoder
force-pushed
the
sideshowcoder/issue/1299
branch
from
a609ca6 to
26df9e7
Compare
Test provider should respect the init-delay during all test Signed-off-by: Philipp Fehre <[email protected]>
With the events being executed on a different thread, we need to wait to make sure the thread is scheduled to have the events emitted. Signed-off-by: Philipp Fehre <[email protected]>
sideshowcoder
force-pushed
the
sideshowcoder/issue/1299
branch
from
26df9e7 to
2b953e0
Compare
|
All code based on work by @chrfwow, based on #1299.
My current understanding is that when stacking event emitting inside an EventProvider, when using sychronization the EventProvider can deadlock, to avoid this move the event emitting of the main thread. Similar to how this is handled in EventSupport.
Still requires cleanup and moving the tests to a better location, I'm still not sure I 100% understand the reason for the deadlock, but wanted to get the work started by porting the testcase provided by @chrfwow.