FFL-1621 Fix: set currentState in a blocking call #3068

typotter · 2025-12-19T07:36:11Z

What does this PR do?

This PR changes the way that the current state's access and mutation is handled to ensure the ordering of state changes delivered to listeners. It also emphasizes the need for usercode listeners to catch their own exceptions lest they bubble and crash the app.

Motivation

As noted in #2998, the currentState was updated synchronously erroneously and prevented other components from realizing that state directly after updating

The decision was made to bubble usercode exceptions in order to fail fast and surface those errors to the dev rather than catching and logging them in our dispatch handler.

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

datadog-official · 2025-12-19T07:56:09Z

🎯 Code Coverage
• Patch Coverage: 100.00%
• Overall Coverage: 66.48% (-4.77%)

View detailed report

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 9d4291c | Docs | Datadog PR Page | Was this helpful? Give us feedback!}

codecov-commenter · 2025-12-19T08:08:16Z

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.50%. Comparing base (17c117e) to head (9d4291c).
⚠️ Report is 38 commits behind head on develop.

Files with missing lines	Patch %	Lines
...in/kotlin/com/datadog/android/flags/FlagsClient.kt	0.00%	1 Missing ⚠️
...atadog/android/flags/internal/FlagsStateManager.kt	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3068      +/-   ##
===========================================
+ Coverage    71.40%   71.50%   +0.10%     
===========================================
  Files          881      881              
  Lines        32460    32471      +11     
  Branches      5466     5475       +9     
===========================================
+ Hits         23178    23218      +40     
+ Misses        7729     7717      -12     
+ Partials      1553     1536      -17

Files with missing lines	Coverage Δ
...in/kotlin/com/datadog/android/flags/FlagsClient.kt	`36.52% <0.00%> (+2.31%)`	⬆️
...atadog/android/flags/internal/FlagsStateManager.kt	`92.31% <87.50%> (+5.94%)`	⬆️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

typotter · 2025-12-19T07:37:43Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

            currentState = newState
            subscription.notifyListeners {
-                onStateChanged(newState)
+                executorService.executeSafe(


enqueue safe executions of the listeners instead of safe execution of the queue. Now listeners cannot not crash the rest

Should we move synchronized protection inside DDCoreSubscriptionImpl?, it seems to me the job of DDCoreSubscriptionImpl to guarantee the thread-safety no matter where it is used.

The DDCoreSubscription ensures atomicity wrt modifying the subscriber list during a notification loop. Our synchronized here is layering atomicity linking the notification mechanics of DDCoreSubscription with mutation of the currentState. This coupling is a specific use case of the DDCoreSubscription or another layer on top.

DDCoreSubscription is still susceptible to listeners crashing the notification loop. We could put a try-catch there, but it might be better to keep the onus on the caller (which easy to do via the block argument passed to notifyListeners.

wdyt?

why do we need do executorService.executeSafe instead of simply calling onStateChanged(newState)?

originally, it was to ensure fairness of state changes - which we've actually lost here with the synchronized block (switched to a ReentrantLock now)

The executor lets us run the subscription consumer off-thread and not block the caller to setEvaluationContext on potential long running consumers. Would it be better to leave the thread mgmt to the caller here? Would the caller need to run their consumer specifically in the UI thread (or some other specific thread?)

Would it be better to leave the thread mgmt to the caller here?

I would say yes, it is up to the caller to do it unless we are picky about listener performance and for us it is critical for it to be fast. And then we can wrap simply onStateChanged calls in try-catch if we want to continue work after consumer threw exception. But then this exception will be handled, so maybe it is actually better to let it propagate and crash the app (since it is exception from the customer code).

I am in favour of the more straightforward approach here where we put onus for threading and exception handling on the user. Fail-fast prevents us from hiding exceptions from the dev and clear docs should point them in the right direction.

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

typotter

Thanks @ambushwork ptal.

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

typotter · 2025-12-19T16:24:27Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

            currentState = newState
            subscription.notifyListeners {
-                onStateChanged(newState)
+                executorService.executeSafe(


The DDCoreSubscription ensures atomicity wrt modifying the subscriber list during a notification loop. Our synchronized here is layering atomicity linking the notification mechanics of DDCoreSubscription with mutation of the currentState. This coupling is a specific use case of the DDCoreSubscription or another layer on top.

DDCoreSubscription is still susceptible to listeners crashing the notification loop. We could put a try-catch there, but it might be better to keep the onus on the caller (which easy to do via the block argument passed to notifyListeners.

wdyt?

0xnm · 2025-12-23T09:48:56Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

+     * The fair parameter ensures that threads acquire the lock in the order they requested it.
+     */


threads acquire the lock in the order they requested it

Does it really matter actually? I'm wondering if it is better to use ReadWriteLock since probably the number of read operations will dominate over the number of write operations, or is it a wrong assumption?

Good point. We could use a ReentrantReadWriteLock to allow reads and still provide ordering for writes (to maintain the ordering of state change notifications sent to listeners).

In practice, I think the direct reads will not be that high, as the use case will typically be to, once on app load:

check the current state

listen to state changes (typically, to then load flag-critical parts of the app when the provider is READY)

At any rate, it makes sense to allow the current state to be read concurrently with notification operations. Thanks!

0xnm · 2025-12-23T09:51:36Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

            currentState = newState
            subscription.notifyListeners {
-                onStateChanged(newState)
+                executorService.executeSafe(


Would it be better to leave the thread mgmt to the caller here?

I would say yes, it is up to the caller to do it unless we are picky about listener performance and for us it is critical for it to be fast. And then we can wrap simply onStateChanged calls in try-catch if we want to continue work after consumer threw exception. But then this exception will be handled, so maybe it is actually better to let it propagate and crash the app (since it is exception from the customer code).

typotter

Happy new year!
Addressed the executor and lock suggestions. PTAL/

typotter · 2026-01-05T16:56:05Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

+     * The fair parameter ensures that threads acquire the lock in the order they requested it.
+     */


Good point. We could use a ReentrantReadWriteLock to allow reads and still provide ordering for writes (to maintain the ordering of state change notifications sent to listeners).

In practice, I think the direct reads will not be that high, as the use case will typically be to, once on app load:

check the current state

listen to state changes (typically, to then load flag-critical parts of the app when the provider is READY)

At any rate, it makes sense to allow the current state to be read concurrently with notification operations. Thanks!

typotter · 2026-01-05T20:36:12Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

            currentState = newState
            subscription.notifyListeners {
-                onStateChanged(newState)
+                executorService.executeSafe(


I am in favour of the more straightforward approach here where we put onus for threading and exception handling on the user. Fail-fast prevents us from hiding exceptions from the dev and clear docs should point them in the right direction.

…manage threads and catch exceptions

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

0xnm · 2026-01-06T10:08:26Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

-        ) {
-            val state = currentState
-            listener.onStateChanged(state)
+        stateLock.write {


in fact both addListener and removeListener don't need to be wrapped in the lock, because DDCoreSubscription is using copy-on-write under the hood, so changes to the subscription won't affect ongoing subscription.notifyListeners call

We want to ensure that the listener gets the current state here before any other state is broadcast, so we lock the current state until done. However, if we notify listener with the current state before adding it to the listeners, we can get by without the lock.

dropped the lock for removeListener as well - DDCoreSubscription is indeed sufficient here

...dk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/FlagsStateManagerTest.kt

Co-authored-by: Nikita Ogorodnikov <4046447+0xnm@users.noreply.github.com>

0xnm

lgtm. Maybe you would like to get also a look from @aleksandr-gringauz before merging.

typotter · 2026-01-06T16:12:51Z

Thanks Nikita!

aleksandr-gringauz · 2026-01-06T17:01:37Z

@codex review

aleksandr-gringauz · 2026-01-06T17:02:14Z

I think I found one small problem (not 100% sure), let's see if codex finds it.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ad003c3de

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-01-06T17:04:28Z

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

    override fun addListener(listener: FlagsStateListener) {
+        listener.onStateChanged(currentState)
        subscription.addListener(listener)


Register listener after state read under lock

In addListener the new listener is invoked before registration and without acquiring stateLock even though currentState is only written under that lock and is no longer @Volatile. If updateState is writing at the same time, this path can read a stale state and deliver it before the listener is ever added, so an in-flight transition (e.g., Ready) is missed and the subscriber remains out of sync. The initial callback should be issued while holding the read lock and after the listener is added to avoid lost updates.

Useful? React with 👍 / 👎.

Yes. This is it.

I think it is better to put these 2 lines under the read-lock:

stateLock.read { listener.onStateChanged(currentState) subscription.addListener(listener) }

Because otherwise in case of concurrent addListener(listener_1) and updateState(new_state) listener_1 might end up not with the latest state in the end.

The order could be the following:

addListener(listener_1) and updateState(state_new) are called concurrently from different threads.

read of currentState occurs in addListener and returns state_old.

updateState does its thing, updating currentState=state_new and notifying listeners. But not listener_1, because it hasn't been added yet.

listener.onStateChanged(state_old) inside addListener is called.

listener_1 ends up with state_old.

Reverting back to the read lock here!

rewrapped the state access + addListener in the read lock plus added a test. ptal

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt

typotter · 2026-01-06T17:51:44Z

pushed a Small fix. PTAL for final merge

typotter · 2026-01-06T18:42:16Z

/merge

dd-devflow-routing-codex · 2026-01-06T18:42:20Z

View all feedbacks in Devflow UI.

2026-01-06 18:42:20 UTC ℹ️ Start processing command /merge

2026-01-06 18:42:26 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in develop is approximately 1h (p90).

2026-01-06 19:48:28 UTC ℹ️ MergeQueue: This merge request was merged

Cherry-picked from PR #3068 to enable OpenFeature provider implementation. Adds state observation capabilities to FlagsClient with thread-safe synchronous state management using ReentrantReadWriteLock. Key changes: - FlagsStateManager: Synchronous state updates with read/write lock - State notification: Listeners called directly (no executor) - Thread safety: Caller manages threading and exception handling - Test updates: Enhanced coverage for state management Original PR: #3068 Branch: typo/FFL-1621-sdk-android

typotter commented Dec 19, 2025

View reviewed changes

typotter changed the title ~~FFL-1621~~ FFL-1621 Fix: set currentState in a blocking call Dec 19, 2025

typotter marked this pull request as ready for review December 19, 2025 08:27

typotter requested a review from a team as a code owner December 19, 2025 08:27

typotter commented Dec 19, 2025

View reviewed changes

typotter requested review from aleksandr-gringauz and ambushwork December 19, 2025 16:27

0xnm reviewed Dec 23, 2025

View reviewed changes

typotter commented Jan 5, 2026

View reviewed changes

typotter mentioned this pull request Jan 5, 2026

[Flags/openfeature] Add openfeature type conversion utilities #3103

Merged

3 tasks

typotter added 9 commits January 5, 2026 15:53

fix: ensure state is set syncronously in the Flag State Manager

38b9ee3

update tests

d52b508

sync remove listener

878d304

use a reentrant lock

ac1437c

safe calls

999a450

use a readwrite lock

7bf3a93

drop use of executor in callback notification; put onus on caller to …

c7cd779

…manage threads and catch exceptions

imports

b1c4839

safe calls

3e50ece

typotter force-pushed the typo/FFL-1621-sdk-android branch from 0e8fb0a to 3e50ece Compare January 5, 2026 23:05

typotter requested a review from 0xnm January 6, 2026 07:50

typotter mentioned this pull request Dec 19, 2025

feat: Openfeature Provider implementation powered by DD Flags #2998

Open

3 tasks

0xnm reviewed Jan 6, 2026

View reviewed changes

typotter and others added 3 commits January 6, 2026 08:53

Apply suggestions from code review

6118ce8

Co-authored-by: Nikita Ogorodnikov <4046447+0xnm@users.noreply.github.com>

fix test

e2e7a11

reduce lock usage

7b9d7ca

0xnm previously approved these changes Jan 6, 2026

View reviewed changes

refine test

3ad003c

typotter dismissed 0xnm’s stale review via 3ad003c January 6, 2026 16:12

chatgpt-codex-connector bot reviewed Jan 6, 2026

View reviewed changes

0xnm reviewed Jan 6, 2026

View reviewed changes

...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/FlagsStateManager.kt Outdated Show resolved Hide resolved

restore lock on addListener and add a test to exercise

316fca4

typotter requested a review from 0xnm January 6, 2026 17:31

aleksandr-gringauz previously approved these changes Jan 6, 2026

View reviewed changes

test imports

9d4291c

typotter dismissed aleksandr-gringauz’s stale review via 9d4291c January 6, 2026 17:45

typotter requested a review from aleksandr-gringauz January 6, 2026 17:51

aleksandr-gringauz approved these changes Jan 6, 2026

View reviewed changes

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Jan 6, 2026

dd-mergequeue bot merged commit 7b2040f into develop Jan 6, 2026
27 checks passed

dd-mergequeue bot deleted the typo/FFL-1621-sdk-android branch January 6, 2026 19:48

dd-devflow bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Jan 6, 2026

typotter added a commit that referenced this pull request Jan 8, 2026

Merge feature/flags-ofeat with merged PRs (#3103, #3104, #3068)

2328698

		* The fair parameter ensures that threads acquire the lock in the order they requested it.
		*/

FFL-1621 Fix: set currentState in a blocking call #3068

FFL-1621 Fix: set currentState in a blocking call #3068

Uh oh!

Conversation

typotter commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

datadog-official bot commented Dec 19, 2025 • edited by datadog-datadog-prod-us1 bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

typotter commented Jan 6, 2026

Uh oh!

aleksandr-gringauz commented Jan 6, 2026

Uh oh!

aleksandr-gringauz commented Jan 6, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

typotter commented Dec 19, 2025 •

edited

Loading

datadog-official bot commented Dec 19, 2025 •

edited by datadog-datadog-prod-us1 bot

Loading

codecov-commenter commented Dec 19, 2025 •

edited

Loading

dd-devflow-routing-codex bot commented Jan 6, 2026 •

edited

Loading