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: 100.00%
• Overall Coverage: 66.40% (+0.03%)

View detailed report

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.30%. Comparing base (b4446a0) to head (869276e).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
...atadog/android/flags/internal/FlagsStateManager.kt	89.47%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3068      +/-   ##
===========================================
- Coverage    71.32%   71.30%   -0.02%     
===========================================
  Files          880      880              
  Lines        32360    32365       +5     
  Branches      5457     5457              
===========================================
- Hits         23079    23076       -3     
- Misses        7733     7746      +13     
+ Partials      1548     1543       -5     

Files with missing lines	Coverage Δ
...atadog/android/flags/internal/FlagsStateManager.kt	92.59% <89.47%> (+6.23%)	⬆️

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

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?

[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).