[flags] FFL-1236 Add CountDownLatch to FlagsRepository#2947
Conversation
083c157 to
a40e2f8
Compare
|
LGTM If Maybe we could simply not specify 100ms at all. The only downside I see is the potential risk of a deadlock in case we made some kind of mistake in our SDK. Another solution is to have some sort of asynchronous api |
typotter
left a comment
typotter
left a comment
There was a problem hiding this comment.
LGTM. Just a question about interrupted threads
| atomicState.compareAndSet(null, loadedState) | ||
| } | ||
| } finally { | ||
| persistenceLoadedLatch.countDown() |
| try { | ||
| persistenceLoadedLatch.await(persistenceLoadTimeoutMs, TimeUnit.MILLISECONDS) | ||
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() |
There was a problem hiding this comment.
InterruptedException is thrown if the countdown latch does not open within the timeout. What is the effect of interrupting the current thread here instead of log and return?
There was a problem hiding this comment.
log and return would swallow the interrupt, but calling Thread.currentThread().interrupt() preserves it for callers further up the stack
I think this will be the best approach we can think of for later, because using |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/feature-flagging #2947 +/- ##
============================================================
- Coverage 70.76% 70.70% -0.07%
============================================================
Files 835 835
Lines 30388 30400 +12
Branches 5132 5132
============================================================
- Hits 21504 21492 -12
- Misses 7431 7448 +17
- Partials 1453 1460 +7
🚀 New features to boost your workflow:
|
5 participants
What does this PR do?
Add a countDownLatch to the
FlagsRepositoryto prevent defaultValue being returned due to a race condition of persistence not finishing loading on startup before a flag is requested. Now the user will either get the persistence value or, after a maximum of 100ms, will get the default.Motivation
This aligns with a change performed on iOS to add a semaphore to the repository there.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)