fix: Move Android SharedPreferences IO off the main thread to prevent ANR - #432
Open
vlaushkin wants to merge 1 commit into
Open
fix: Move Android SharedPreferences IO off the main thread to prevent ANR#432vlaushkin wants to merge 1 commit into
vlaushkin wants to merge 1 commit into
Conversation
|
To preview the documentation for this pull request, visit the following URL: docs.page/abausg/home_widget~432
|
Walkthrough
ChangesAsync SharedPreferences via Kotlin coroutines
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt (3)
75-75: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider passing message to exception constructor.
Static analysis suggests passing the error message to the
IllegalArgumentExceptionconstructor rather than using the default constructor. While the message is provided toresult.error(), passing it to the exception constructor improves stack trace readability.📝 Suggested improvement
result.error( "-10", - "Invalid Type ${data!!::class.java.simpleName}. Supported types are Boolean, Float, String, Double, Long", - IllegalArgumentException(), + "Invalid Type ${data!!::class.java.simpleName}. Supported types are Boolean, Float, String, Double, Long, Int", + IllegalArgumentException("Unsupported data type: ${data!!::class.java.simpleName}"), )Note: Also added "Int" to the supported types list in the message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt` at line 75, The IllegalArgumentException() constructor call is missing an error message argument. Modify the IllegalArgumentException() instantiation to pass the error message to the constructor instead of using the default constructor. The error message should be the same descriptive message that is being passed to the result.error() call, which will improve stack trace readability and provide better context for debugging.Source: Linters/SAST tools
101-113: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider exception handling for read failures.
The pattern correctly moves SharedPreferences reads to the IO dispatcher (first access may load the backing file). However, similar to
saveWidgetData, exceptions during the read operation could preventresult.success()from being called.🛡️ Optional: Add exception handling
coroutineScope.launch { - val value = - withContext(ioDispatcher) { - val prefs = context.getSharedPreferences(PREFERENCES, Context.MODE_PRIVATE) - val stored = prefs.all[id] ?: defaultValue - if (stored is Long && prefs.getBoolean("$doubleLongPrefix$id", false)) { - java.lang.Double.longBitsToDouble(stored) - } else { - stored - } - } - result.success(value) + try { + val value = + withContext(ioDispatcher) { + val prefs = context.getSharedPreferences(PREFERENCES, Context.MODE_PRIVATE) + val stored = prefs.all[id] ?: defaultValue + if (stored is Long && prefs.getBoolean("$doubleLongPrefix$id", false)) { + java.lang.Double.longBitsToDouble(stored) + } else { + stored + } + } + result.success(value) + } catch (e: Exception) { + result.error("-12", "Failed to read preferences: ${e.message}", e) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt` around lines 101 - 113, The coroutineScope.launch block that reads from SharedPreferences using withContext(ioDispatcher) lacks exception handling, which means any exceptions during the read operation will prevent result.success() from being called and leave the request hanging. Wrap the withContext(ioDispatcher) block in a try-catch statement, and in the catch block, call result.error() with an appropriate error code and error message to properly handle read failures, similar to the error handling pattern used in saveWidgetData.
84-87: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider exception handling for commit failures.
While
commit()typically returnsfalseon failure (which is correctly propagated to the caller), it could theoretically throw exceptions in severe conditions (e.g., out of memory). If an exception is thrown withinwithContext, the coroutine will fail andresult.success()will never be called, leading to "reply never submitted" on the Dart side.🛡️ Optional: Add exception handling
coroutineScope.launch { - val committed = withContext(ioDispatcher) { prefs.commit() } - result.success(committed) + try { + val committed = withContext(ioDispatcher) { prefs.commit() } + result.success(committed) + } catch (e: Exception) { + result.error("-11", "Failed to commit preferences: ${e.message}", e) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt` around lines 84 - 87, The coroutineScope.launch block that calls prefs.commit() within withContext lacks exception handling. If an exception is thrown during the commit operation, the coroutine will fail silently and result.success() will never be invoked, causing "reply never submitted" errors on the Dart side. Wrap the withContext block in a try-catch statement to handle any exceptions that might be thrown by prefs.commit() or the context switching operation, and call result.error() in the catch block with appropriate error details to ensure the Dart side always receives a response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt`:
- Line 75: The IllegalArgumentException() constructor call is missing an error
message argument. Modify the IllegalArgumentException() instantiation to pass
the error message to the constructor instead of using the default constructor.
The error message should be the same descriptive message that is being passed to
the result.error() call, which will improve stack trace readability and provide
better context for debugging.
- Around line 101-113: The coroutineScope.launch block that reads from
SharedPreferences using withContext(ioDispatcher) lacks exception handling,
which means any exceptions during the read operation will prevent
result.success() from being called and leave the request hanging. Wrap the
withContext(ioDispatcher) block in a try-catch statement, and in the catch
block, call result.error() with an appropriate error code and error message to
properly handle read failures, similar to the error handling pattern used in
saveWidgetData.
- Around line 84-87: The coroutineScope.launch block that calls prefs.commit()
within withContext lacks exception handling. If an exception is thrown during
the commit operation, the coroutine will fail silently and result.success() will
never be invoked, causing "reply never submitted" errors on the Dart side. Wrap
the withContext block in a try-catch statement to handle any exceptions that
might be thrown by prefs.commit() or the context switching operation, and call
result.error() in the catch block with appropriate error details to ensure the
Dart side always receives a response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64b17785-b8dd-4d0e-863c-da54ead80265
📒 Files selected for processing (1)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetPlugin.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
On Android,
HomeWidget.saveWidgetDatapersisted data by callingSharedPreferences.Editor.commit()directly insideonMethodCall, which runs on the platform (main) thread.commit()performs a synchronousfsync, so on slow storage it blocks the UI thread and triggers ANRs. This is exactly the stack reported in #401:This PR moves the blocking
SharedPreferencesdisk IO off the platform thread:saveWidgetDatanow runscommit()off the platform thread and replies once the write finishes. The reply is still sent only after the write completes, so an awaitedsaveWidgetDataFuture continues to guarantee the data has been persisted — the existing save-then-read ordering (and the integration tests that rely on it) keep working.getWidgetDataresolves its value off the platform thread as well, since the first read loads the backing file from disk.Dispatchers.IO.limitedParallelism(1), i.e. a single-threaded view of the IO pool. This keeps the previous behaviour where operations were serialized in call order (so concurrent/non-awaited reads and writes can't reorder), while no longer blocking the platform thread.CoroutineScope(SupervisorJob() + Dispatchers.Main)that is cancelled inonDetachedFromEngine.kotlinx-coroutines-androidis already a dependency of the plugin.While restructuring the
saveWidgetDatabranch I also fixed a latent double-reply: for an unsupported data type the handler calledresult.error(...)and then still fell through toresult.success(prefs.commit()), submitting two replies for one call. The unsupported-type branch nowreturns afterresult.error(...). This removes one cause of the "Reply already submitted" crash tracked in #265.Checklist
exampleor documentation.Breaking Change?
Related Issues
Closes #401