Termux-Ausgabe: Wechsel von apply() zu commit() zur Vermeidung einer Race-Condition#105
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR attempts to fix a race condition by switching from apply() to commit() in SharedPreferences operations. While the race condition concern is valid, the implementation introduces critical risks that block merge:
Critical Issues Found
-
ANR Risk: The synchronous
commit()calls perform blocking I/O on the calling thread. If these methods are invoked from the main thread, this will freeze the UI and may trigger Application Not Responding (ANR) dialogs. -
Crash on Storage Failures: Throwing
IllegalStateExceptionwhencommit()returnsfalsewill crash the app. The method can legitimately fail in non-exceptional scenarios (disk full, permissions issues, device shutdown), trading silent data loss for guaranteed crashes.
Recommended Approach
Consider one of these alternatives:
- Move the
commit()calls to a background thread/coroutine to avoid blocking the UI - Use proper synchronization mechanisms with
apply()(e.g., locks or atomic operations) - Handle commit failures gracefully with logging and error indicators rather than crashing
- Evaluate if the race condition frequency justifies the performance trade-off
The current implementation replaces a potential data loss issue with guaranteed UI freezing and app crashes, which is a regression.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| val existing = prefs.getString(KEY_PENDING_OUTPUT, "").orEmpty() | ||
| val merged = if (existing.isBlank()) output else "$existing\n\n$output" | ||
| prefs.edit().putString(KEY_PENDING_OUTPUT, merged).apply() | ||
| val committed = prefs.edit().putString(KEY_PENDING_OUTPUT, merged).commit() |
There was a problem hiding this comment.
🛑 Crash Risk: The synchronous commit() call performs blocking I/O on the calling thread. If appendOutput is called from the main thread, this will freeze the UI and may trigger ANR (Application Not Responding) dialogs, degrading user experience. Consider using a background thread with commit() or keeping apply() with proper synchronization mechanisms to avoid the race condition without blocking the UI thread.
| if (!committed) { | ||
| throw IllegalStateException("Failed to persist pending Termux output") | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Throwing IllegalStateException on commit failure will crash the app. commit() can legitimately return false in non-exceptional scenarios (disk full, storage permissions issues, device shutting down). This replaces a silent data loss issue with guaranteed app crashes. Consider logging the error and returning a failure indicator instead, or using an alternative synchronization strategy that doesn't crash on storage failures.
| val value = prefs.getString(KEY_PENDING_OUTPUT, "").orEmpty().trim() | ||
| if (value.isBlank()) return null | ||
| prefs.edit().remove(KEY_PENDING_OUTPUT).apply() | ||
| val committed = prefs.edit().remove(KEY_PENDING_OUTPUT).commit() |
There was a problem hiding this comment.
🛑 Crash Risk: The synchronous commit() call performs blocking I/O on the calling thread. If consumeOutput is called from the main thread, this will freeze the UI and may trigger ANR (Application Not Responding) dialogs. Consider using a background thread with commit() or an alternative approach that doesn't block the UI thread.
| if (!committed) { | ||
| throw IllegalStateException("Failed to clear consumed Termux output") | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Throwing IllegalStateException on commit failure will crash the app instead of handling storage errors gracefully. commit() can return false in legitimate scenarios like disk full or permissions issues. This trades silent data loss for guaranteed crashes. Consider logging the error and handling it gracefully, or returning a failure indicator to the caller.
1 participant
Motivation
SharedPreferences.apply()asynchron schreibt und die nächste "Screen elements"-Bubble vor dem Persistieren lesen kann, wodurch die Termux-Ausgabe nicht injiziert wird.Description
app/src/main/kotlin/com/google/ai/sample/util/TermuxOutputPreferences.ktwurdeappendOutputso geändert, dass esSharedPreferences.edit().putString(...).commit()stattapply()verwendet.consumeOutputso geändert, dass das Entfernen mitcommit()stattapply()erfolgt.commit()wird nun eineIllegalStateExceptiongeworfen, um stille Persistenzfehler sichtbar zu machen.Testing
./gradlew :app:lintDebug— fehlgeschlagen wegen bestehender, repo-weiter Lint-Fehler (z.B.ProtectedPermissionsinAndroidManifest.xml), die nicht durch diesen Patch verursacht wurden../gradlew :app:compileDebugKotlin— erfolgreich.Codex Task