Skip to content

dataconnect: Improve usage of MutableStateFlow to improve readability #6840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Apr 4, 2025

Use MutableStateFlow.update() instead of MutableStateFlow.compareAndSet() directly for improved readability and less potential for bugs

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 4, 2025

Coverage Report 1

Affected Products

  • firebase-dataconnect

    Overall coverage changed from 14.45% (e98dd2c) to 14.31% (2b86ff6) by -0.15%.

    FilenameBase (e98dd2c)Merge (2b86ff6)Diff
    DataConnectCredentialsTokenManager.kt34.12%30.77%-3.35%
    FirebaseDataConnectImpl.kt42.34%43.05%+0.71%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/jMY06g0QSv.html

Copy link
Contributor

github-actions bot commented Apr 4, 2025

Test Results

   66 files   -    968     66 suites   - 968   1m 19s ⏱️ - 33m 16s
  552 tests  -  5 322    551 ✅  -  5 301  1 💤  - 21  0 ❌ ±0 
1 104 runs   - 10 707  1 102 ✅  - 10 665  2 💤  - 42  0 ❌ ±0 

Results for commit b1f01b0. ± Comparison against base commit e98dd2c.

This pull request removes 5322 tests.
com.google.android.datatransport.cct.CctBackendFactoryTest ‑ create_returnCCTBackend_WhenBackendNameIsCCT
com.google.android.datatransport.cct.CctDestinationTest ‑ cctDestination_shouldOnlySupportProtoAndJson
com.google.android.datatransport.cct.CctDestinationTest ‑ cctDestination_shouldSupportProtoAndJson
com.google.android.datatransport.cct.CctTransportBackendTest ‑ decorate_whenOffline_shouldProperlyPopulateNetworkInfo
com.google.android.datatransport.cct.CctTransportBackendTest ‑ decorate_whenOnline_shouldProperlyPopulateNetworkInfo
com.google.android.datatransport.cct.CctTransportBackendTest ‑ schedule_shouldAddCookieOnPseudonymousIds
com.google.android.datatransport.cct.CctTransportBackendTest ‑ schedule_shouldDropCookieOnMixedPseudonymousIds
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_CompressedResponseIsUncompressed
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_whenBackendRedirectsMoreThan5Times_shouldOnlyRedirect4Times
com.google.android.datatransport.cct.CctTransportBackendTest ‑ send_whenBackendRedirects_shouldCorrectlyFollowTheRedirectViaPost
…

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 4, 2025

Size Report 1

Affected Products

  • firebase-dataconnect

    TypeBase (e98dd2c)Merge (2b86ff6)Diff
    aar709 kB710 kB+835 B (+0.1%)
    apk (release)10.0 MB10.0 MB+832 B (+0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/0Z8vglpmOn.html

Base automatically changed from dconeybe/dataconnect/AuthCodeCleanup to main April 8, 2025 21:07
Copy link
Contributor

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_responses.sh should be updated to clone the latest version of the responses: v10.0

@dconeybe dconeybe requested a review from Copilot April 16, 2025 17:42
@dconeybe dconeybe marked this pull request as ready for review April 16, 2025 17:42
Copy link
Contributor

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:196

  • [nitpick] Consider renaming the lambda parameter 'oldState' to 'currentState' in the forceRefresh function for improved clarity, as it represents the prior state before the update.
state.getAndUpdate { oldState ->

firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:343

  • [nitpick] Consider renaming the lambda parameter 'oldState' to 'currentState' in the onProviderAvailable function for consistency and clearer indication that it represents the pre-update state.
state.getAndUpdate { oldState ->

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:195

  • In forceRefresh(), the previous implementation checked that the new state had forceTokenRefresh set to true. Please verify that the removal of this explicit check in the getAndUpdate update block does not allow for unexpected state transitions.
val oldState = state.getAndUpdate { currentState ->

firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt:342

  • In onProviderAvailable(), the CAS loop is replaced with a getAndUpdate call that always returns the old state. Confirm that this change still satisfies the intended concurrency and state transition guarantees.
val oldState = state.getAndUpdate { currentState ->

…efresh() to avoid unintentional internal state corruption.

This was suggested by Copilot: #6840 (review)
@dconeybe dconeybe requested a review from Copilot April 16, 2025 18:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances readability and reduces the potential for bugs by replacing explicit CAS loops with the more concise and expressive MutableStateFlow update helpers. Key changes include:

  • Replacing while loops using compareAndSet with update(), updateAndGet(), and getAndUpdate().
  • Simplifying asynchronous state management in the close job and token management flows.
  • Improving clarity of state transitions in multiple components.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/SuspendingCountDownLatch.kt Replaced CAS loop with update() for decrementing the count.
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/querymgr/RegisteredDataDeserialzer.kt Replaced CAS loop with update() for atomically setting the latest update.
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/QuerySubscriptionImpl.kt Replaced CAS loop with update() to safely update the last result.
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt Replaced CAS loop with updateAndGet() to manage the close job state more robustly.
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt Replaced CAS loops with getAndUpdate() to handle state transitions during close, force refresh, and token provider updates.

@dconeybe dconeybe merged commit 51b4a1c into main Apr 16, 2025
42 checks passed
@dconeybe dconeybe deleted the dconeybe/dataconnect/MutableStateFlowUseUpdateInsteadOfCompareAndSet branch April 16, 2025 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants