♻️ [KMP] Refactor and simplify refresh token login in AuthProviderImpl#47
Conversation
Summary of ChangesHello @JuliaJakubcova, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the token refresh mechanism within the authentication provider. The primary goal was to simplify the existing logic, which was overly complex and prone to issues related to coroutine scope cancellation. The changes introduce a more robust and streamlined approach to ensure reliable token refreshing across the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughReplaces Deferred-based in-flight token refresh with a mutex-guarded sequential refresh in AuthProviderImpl; refreshToken captures the current token, acquires a Mutex, conditionally refreshes inside withContext(NonCancellable), updates the token, and returns the (possibly refreshed) token. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the token refresh logic by replacing a complex, Deferred-based implementation with a more robust and simpler approach using a Mutex and NonCancellable context. This correctly addresses the issue of caller scope cancellation affecting the refresh operation. I have one suggestion to further refine the new implementation by removing a redundant lock and adopting a more idiomatic return pattern, which will enhance both performance and code clarity.
| val oldToken = mutex.withLock { token } | ||
|
|
||
| val task = async { | ||
| val fresh = tokenRefresher.refresh() | ||
| token = fresh | ||
| fresh | ||
| } | ||
| inFlight = task | ||
| task | ||
| }.let { task -> | ||
| try { | ||
| task.await() | ||
| } finally { | ||
| mutex.withLock { | ||
| if (inFlight === task) { | ||
| inFlight = null | ||
| } | ||
| mutex.withLock { | ||
| if (oldToken == token) { | ||
| withContext(NonCancellable) { | ||
| token = tokenRefresher.refresh() | ||
| } | ||
| } | ||
| return token | ||
| } |
There was a problem hiding this comment.
This implementation of the double-checked locking pattern can be slightly improved for efficiency and readability.
Currently, you are locking the mutex twice: once to read the oldToken and a second time for the check-and-set operation. The first lock is redundant. Reading the token property is not the operation that needs to be synchronized; the critical section is the part that checks the token and potentially refreshes it.
You can simplify this by reading oldToken without a lock, and then using a single withLock block that returns the final token value. This avoids the extra lock/unlock overhead and also makes the code more idiomatic by avoiding a non-local return.
| val oldToken = mutex.withLock { token } | |
| val task = async { | |
| val fresh = tokenRefresher.refresh() | |
| token = fresh | |
| fresh | |
| } | |
| inFlight = task | |
| task | |
| }.let { task -> | |
| try { | |
| task.await() | |
| } finally { | |
| mutex.withLock { | |
| if (inFlight === task) { | |
| inFlight = null | |
| } | |
| mutex.withLock { | |
| if (oldToken == token) { | |
| withContext(NonCancellable) { | |
| token = tokenRefresher.refresh() | |
| } | |
| } | |
| return token | |
| } | |
| val oldToken = token | |
| return mutex.withLock { | |
| if (oldToken == token) { | |
| withContext(NonCancellable) { | |
| token = tokenRefresher.refresh() | |
| } | |
| } | |
| token | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt`:
- Around line 23-34: The first mutex.withLock in refreshToken() is redundant;
collapse the two critical sections into a single mutex.withLock in the
refreshToken function: inside the single withLock read a local oldToken = token,
if oldToken == token then call tokenRefresher.refresh() (using NonCancellable
context) and assign token, then return token; remove the separate initial
mutex.withLock and ensure you still use mutex, token, tokenRefresher and
NonCancellable as in the original.
JuliaJakubcova
force-pushed
the
fix/kmp/auth-token-refresh
branch
from
431cb00 to
428cb7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt`:
- Around line 27-29: The refresh result from tokenRefresher.refresh() may be
null, causing token to be set to null and every subsequent caller to repeatedly
attempt refresh; change the logic inside AuthProviderImpl where token is
assigned (the withContext(NonCancellable) block around tokenRefresher.refresh())
to handle a null return: if refresh() returns null, do not assign null to token
— instead clear stored credentials / set a "needsLogin" flag or throw a specific
AuthenticationRequiredException so callers will stop retrying, persist or
broadcast the logged-out state as appropriate, and ensure the lock-release path
communicates that no further automatic retries should occur until explicit
re-login.
| if (oldToken == token) { | ||
| withContext(NonCancellable) { | ||
| token = tokenRefresher.refresh() |
There was a problem hiding this comment.
Consider the null refresh result scenario.
If tokenRefresher.refresh() returns null (e.g. refresh token expired), token is set to null. Every subsequent caller will snapshot oldToken = null, enter the lock, see null == null → true, and re-attempt the refresh — potentially hammering the auth server.
If this is a realistic failure mode, consider short-circuiting when the refresh yields null (e.g. clear credentials and force re-login rather than retrying indefinitely).
🤖 Prompt for AI Agents
In
`@shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt`
around lines 27 - 29, The refresh result from tokenRefresher.refresh() may be
null, causing token to be set to null and every subsequent caller to repeatedly
attempt refresh; change the logic inside AuthProviderImpl where token is
assigned (the withContext(NonCancellable) block around tokenRefresher.refresh())
to handle a null return: if refresh() returns null, do not assign null to token
— instead clear stored credentials / set a "needsLogin" flag or throw a specific
AuthenticationRequiredException so callers will stop retrying, persist or
broadcast the logged-out state as appropriate, and ensure the lock-release path
communicates that no further automatic retries should occur until explicit
re-login.
JuliaJakubcova
force-pushed
the
fix/kmp/auth-token-refresh
branch
from
428cb7d to
3f39c70
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt (1)
27-30:⚠️ Potential issue | 🟠 MajorHandle
nullrefresh results as a terminal auth state.On Line 29, assigning
tokenRefresher.refresh()directly totokenallowsnullto persist and can cause repeated automatic refresh attempts after terminal failure (e.g., expired refresh token). Stop retrying until explicit re-login (or throw a dedicated auth-required exception).♻️ Suggested hardening
internal class AuthProviderImpl( private val settings: Settings, private val tokenRefresher: TokenRefresher, ) : AuthProvider { + private var requiresLogin = false override var token: String? get() = settings.getStringOrNull(TOKEN_KEY) - set(value) = settings.set(TOKEN_KEY, value) + set(value) { + settings.set(TOKEN_KEY, value) + if (value != null) requiresLogin = false + } private val mutex = Mutex() override suspend fun refreshToken(): String? { val oldToken = token mutex.withLock { + if (requiresLogin) return null if (oldToken == token) { withContext(NonCancellable) { - token = tokenRefresher.refresh() + val refreshed = tokenRefresher.refresh() + if (refreshed == null) { + requiresLogin = true + token = null + return@withContext + } + token = refreshed } } return token } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt` around lines 27 - 30, The current refresh logic in AuthProviderImpl assigns tokenRefresher.refresh() directly to token which allows a null to persist and triggers repeated refresh attempts; change the block inside the oldToken == token branch so that after calling tokenRefresher.refresh() you check the result and if it is null treat it as a terminal auth state: do not assign null to token and instead either set an explicit unauthenticated flag or throw a dedicated AuthRequiredException (e.g., throw AuthRequiredException() from the refresh path) so callers stop retrying and require an explicit re-login; update the code around the withContext(NonCancellable) { token = tokenRefresher.refresh() } call in AuthProviderImpl to perform this null-check and take the terminal action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt`:
- Around line 27-30: The current refresh logic in AuthProviderImpl assigns
tokenRefresher.refresh() directly to token which allows a null to persist and
triggers repeated refresh attempts; change the block inside the oldToken ==
token branch so that after calling tokenRefresher.refresh() you check the result
and if it is null treat it as a terminal auth state: do not assign null to token
and instead either set an explicit unauthenticated flag or throw a dedicated
AuthRequiredException (e.g., throw AuthRequiredException() from the refresh
path) so callers stop retrying and require an explicit re-login; update the code
around the withContext(NonCancellable) { token = tokenRefresher.refresh() } call
in AuthProviderImpl to perform this null-check and take the terminal action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 569e5153-aa30-4ffc-8caa-1568df41d64c
📒 Files selected for processing (1)
shared/auth/src/commonMain/kotlin/kmp/shared/auth/data/provider/AuthProviderImpl.kt
3 participants
📝 Description
💡 What’s new?
😶 What’s missing?
Summary by CodeRabbit