FFL-1621 Fix: set currentState in a blocking call

[typotter]

What does this PR do?

This PR changes the way that the current state's access and mutation is handled.

• Listener blocks are wrapped in executeSafe to avoid crashing the pool before the rest of the listeners are notified.
• the updatetState and calls to executeSafe are made in a sync block
• addListener now does work inside a synchronized(currentState) block as well to ensure atomic registration+currentState notifcation
• callback in addListener also runs in an executeSafe block

Motivation

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

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]

🎯 Code Coverage
• Patch Coverage: 69.23%
• Overall Coverage: 66.47% (-4.78%)

View detailed report

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 3e50ece | Docs | Datadog PR Page | Was this helpful? Give us feedback!}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.43%. Comparing base (17c117e) to head (3e50ece).
⚠️ Report is 2 commits behind head on develop.

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

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3068      +/-   ##
===========================================
+ Coverage    71.40%   71.43%   +0.03%     
===========================================
  Files          881      881              
  Lines        32460    32472      +12     
  Branches      5466     5477      +11     
===========================================
+ Hits         23178    23195      +17     
+ Misses        7729     7725       -4     
+ Partials      1553     1552       -1     

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	78.57% <70.00%> (-7.79%)	⬇️

... and 38 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]

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

[ambushwork]

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.

[typotter]

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?

[aleksandr-gringauz]

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

[typotter]

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?)

[0xnm]

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]

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.

[typotter]

Thanks @ambushwork ptal.

[typotter]

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]

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?

[typotter]

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:

1. check the current state
2. 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]

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]

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:

1. check the current state
2. 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]

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.