feat: Add direct background execution mode bypassing WorkManager#406
feat: Add direct background execution mode bypassing WorkManager#406armandsLa wants to merge 2 commits into
Conversation
|
To view this pull requests documentation preview, visit the following URL: docs.page/abausg/home_widget~406 Documentation is deployed and generated using docs.page. |
WalkthroughBroadcast receiver now conditionally routes background intents using an intent extra Changes
Sequence Diagram(s)sequenceDiagram
participant Broadcast as BroadcastReceiver
participant Runner as HomeWidgetBackgroundRunner
participant Engine as FlutterEngine
participant Channel as MethodChannel
participant Main as MainThread
Broadcast->>Runner: execute(context, intent, pendingResult)
activate Runner
Runner->>Runner: launch coroutine (background)
alt Engine not initialized
Runner->>Engine: initialize FlutterEngine using dispatcher handle
Engine->>Channel: create channel "home_widget/background" and set handler
end
Runner->>Runner: prepare args [handle, data] and enqueue or dispatch
Runner->>Main: post invocation to main thread
Main->>Channel: invokeMethod("", args, callback)
Channel->>Runner: invokeResult -> pendingResult.finish()
deactivate Runner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 115 115
=========================================
Hits 115 115 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt (2)
48-48: Empty method name ininvokeMethod("")appears intentional but should be documented.Using an empty string as the method name is unconventional. If this is the expected protocol for the Dart side's "home_widget/background" channel, please add a brief comment explaining this design choice for maintainability.
📝 Suggested documentation
+ // The Dart background channel expects an empty method name with [callbackHandle, data] as arguments mainHandler.post { channel?.invokeMethod("", args) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt` at line 48, The call mainHandler.post { channel?.invokeMethod("", args) } in HomeWidgetBackgroundRunner.kt uses an empty string for the method name which is unconventional; add a brief inline comment immediately above the mainHandler.post (or next to channel?.invokeMethod) explaining that an empty string is the expected protocol for the Dart-side "home_widget/background" channel and why (e.g., used as a signal/message-only invocation), so future maintainers understand the intentional design choice; keep the comment short and reference the channel name "home_widget/background" and the invokeMethod call.
78-100: Engine initialization exceptions will crash the coroutine butpendingResult.finish()is still called - verify exception propagation.The
initializeFlutterEnginemethod throwsIllegalStateExceptionif the callback handle is missing or lookup fails. These exceptions will propagate up and be caught by thefinallyblock, ensuringpendingResult.finish()is called. This is correct behavior.However, if
FlutterEngine(context)orexecuteDartCallbackthrows on the main thread (line 91, 98), the exception will propagate throughwithContext(Dispatchers.Main)and terminate the coroutine. Consider whether you want to log these errors or handle them more gracefully.📝 Suggested error handling
private suspend fun initializeFlutterEngine(context: Context) { val callbackHandle = HomeWidgetPlugin.getDispatcherHandle(context) if (callbackHandle == 0L) { - throw IllegalStateException( - "No callbackHandle saved. Did you call HomeWidget.registerBackgroundCallback?" - ) + android.util.Log.e("HomeWidgetBackgroundRunner", + "No callbackHandle saved. Did you call HomeWidget.registerBackgroundCallback?") + return } val callbackInfo = FlutterCallbackInformation.lookupCallbackInformation(callbackHandle) - ?: throw IllegalStateException("Failed to lookup callback information") + if (callbackInfo == null) { + android.util.Log.e("HomeWidgetBackgroundRunner", "Failed to lookup callback information") + return + } withContext(Dispatchers.Main) { + try { engine = FlutterEngine(context) // ... + } catch (e: Exception) { + android.util.Log.e("HomeWidgetBackgroundRunner", "Failed to initialize FlutterEngine", e) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt` around lines 78 - 100, The initializeFlutterEngine function can throw on the main thread when constructing FlutterEngine or executing the Dart callback; wrap the critical part inside withContext(Dispatchers.Main) in a try/catch that catches Throwable, log the error (including the exception and context such as the callbackHandle and callbackInfo) and handle it gracefully (e.g., leave engine null, avoid rethrowing if you want pendingResult.finish() to run normally, or rethrow after logging if preferred). Specifically, surround the FlutterEngine(context) creation and engine?.dartExecutor?.executeDartCallback(callback) calls with a catch block, use your module logger or Android Log.e to record the exception and relevant identifiers (callbackHandle, callbackInfo), and ensure the rest of the coroutine/ finally block (pendingResult.finish()) behaves according to your chosen recovery strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt`:
- Around line 51-54: The current finally block calls pendingResult.finish()
before the Dart callback completes; modify HomeWidgetBackgroundRunner.kt so
pendingResult is not finished in the finally block but passed through to the
posted Runnable and finished from the MethodChannel.Result callback of
channel?.invokeMethod (or an equivalent callback) after
success/error/notImplemented, e.g., post the invokeMethod on mainHandler and
call pendingResult.finish() inside each Result override; remove the premature
finish in finally and ensure pendingResult is accessible to the invoked method
call so Dart completion confirms before finishing.
- Around line 33-40: The code has a race where concurrent execute() calls can
both see engine == null and call initializeFlutterEngine(), and channel is
written on Dispatchers.Default but read on main thread causing visibility
issues; fix by adding a dedicated init lock (e.g., private val initLock = Any())
and an ensureEngineInitialized(context) helper (called from execute()) that does
a double-checked pattern: if (engine != null) return; synchronized(initLock) {
if (engine != null) return; initializeFlutterEngine(context) } and mark shared
vars like engine and channel as `@Volatile` to ensure visibility across threads
(references: HomeWidgetBackgroundRunner, execute, initializeFlutterEngine,
engine, channel, initLock, ensureEngineInitialized).
---
Nitpick comments:
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt`:
- Line 48: The call mainHandler.post { channel?.invokeMethod("", args) } in
HomeWidgetBackgroundRunner.kt uses an empty string for the method name which is
unconventional; add a brief inline comment immediately above the
mainHandler.post (or next to channel?.invokeMethod) explaining that an empty
string is the expected protocol for the Dart-side "home_widget/background"
channel and why (e.g., used as a signal/message-only invocation), so future
maintainers understand the intentional design choice; keep the comment short and
reference the channel name "home_widget/background" and the invokeMethod call.
- Around line 78-100: The initializeFlutterEngine function can throw on the main
thread when constructing FlutterEngine or executing the Dart callback; wrap the
critical part inside withContext(Dispatchers.Main) in a try/catch that catches
Throwable, log the error (including the exception and context such as the
callbackHandle and callbackInfo) and handle it gracefully (e.g., leave engine
null, avoid rethrowing if you want pendingResult.finish() to run normally, or
rethrow after logging if preferred). Specifically, surround the
FlutterEngine(context) creation and
engine?.dartExecutor?.executeDartCallback(callback) calls with a catch block,
use your module logger or Android Log.e to record the exception and relevant
identifiers (callbackHandle, callbackInfo), and ensure the rest of the
coroutine/ finally block (pendingResult.finish()) behaves according to your
chosen recovery strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2cb7aea-a98d-4f85-bbe8-d0fec9b8c712
📒 Files selected for processing (3)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundReceiver.ktpackages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.ktpackages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetIntent.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt (1)
111-148: Engine lifecycle consideration.The
FlutterEngineis created once and never destroyed. While this provides faster response for subsequent broadcasts, it may consume memory if no further widget interactions occur. Consider whether adding a cleanup mechanism (e.g., destroy engine after a timeout of inactivity) would be beneficial for resource-constrained devices.This is not a blocking concern for the current implementation—keeping the engine warm is a reasonable tradeoff for widget responsiveness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt` around lines 111 - 148, initializeFlutterEngine currently creates a FlutterEngine and never releases it (engine and channel stay alive), which can leak memory on idle devices; add a cleanup mechanism by implementing a destroy/cleanup method (e.g., destroyEngine or shutdownEngine) that calls engine.destroy() and clears channel and engine references, and wire it to a timeout/inactivity scheduler (use a CoroutineScope with a delayed job or Handler) started after engine initialization in initializeFlutterEngine and cancelled when the engine is reused; ensure the timeout duration is configurable and that destroyEngine is also called from any explicit lifecycle teardown points to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt`:
- Around line 45-46: The code uses HomeWidgetPlugin.getHandle(context) without
validating its return value; update HomeWidgetBackgroundRunner (before building
args/listOf(...)) to check the handle returned by
HomeWidgetPlugin.getHandle(context) and if it equals 0L, log or abort the
background invocation and return early instead of constructing args and invoking
Dart; ensure you reference the handle variable (e.g., val handle =
HomeWidgetPlugin.getHandle(context)) and only proceed to create args =
listOf(handle, data) and call the dispatcher when handle != 0L.
---
Nitpick comments:
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt`:
- Around line 111-148: initializeFlutterEngine currently creates a FlutterEngine
and never releases it (engine and channel stay alive), which can leak memory on
idle devices; add a cleanup mechanism by implementing a destroy/cleanup method
(e.g., destroyEngine or shutdownEngine) that calls engine.destroy() and clears
channel and engine references, and wire it to a timeout/inactivity scheduler
(use a CoroutineScope with a delayed job or Handler) started after engine
initialization in initializeFlutterEngine and cancelled when the engine is
reused; ensure the timeout duration is configurable and that destroyEngine is
also called from any explicit lifecycle teardown points to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6bd0a9f9-b91b-4b81-9c96-e5427226cb43
📒 Files selected for processing (1)
packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt
| val data = intent.data?.toString() ?: "" | ||
| val args = listOf(HomeWidgetPlugin.getHandle(context), data) |
There was a problem hiding this comment.
Missing validation of getHandle() return value.
Per the relevant code snippet, HomeWidgetPlugin.getHandle(context) returns 0L if the callback was never registered or after app data is cleared. While getDispatcherHandle() is validated in initializeFlutterEngine(), getHandle() is not. Passing 0L to the Dart side will cause the callback lookup to fail silently.
Consider adding validation:
🛡️ Proposed fix
val data = intent.data?.toString() ?: ""
+ val handle = HomeWidgetPlugin.getHandle(context)
+ if (handle == 0L) {
+ Log.w(TAG, "No callback handle registered. Ignoring background event.")
+ pendingResult.finish()
+ return@launch
+ }
- val args = listOf(HomeWidgetPlugin.getHandle(context), data)
+ val args = listOf(handle, data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/home_widget/android/src/main/kotlin/es/antonborri/home_widget/HomeWidgetBackgroundRunner.kt`
around lines 45 - 46, The code uses HomeWidgetPlugin.getHandle(context) without
validating its return value; update HomeWidgetBackgroundRunner (before building
args/listOf(...)) to check the handle returned by
HomeWidgetPlugin.getHandle(context) and if it equals 0L, log or abort the
background invocation and return early instead of constructing args and invoking
Dart; ensure you reference the handle variable (e.g., val handle =
HomeWidgetPlugin.getHandle(context)) and only proceed to create args =
listOf(handle, data) and call the dispatcher when handle != 0L.
|
Can you say a bit more on what the functionality of the change is? Also did you check that this mode works in all scenarios of the app? E.g App is running, App is fully closed, App is in background mode. Also could this potentially be combined with the existing work manager and then skipping the queue there and do the work directly? Wondering if this could be combined for two reasons:
Thanks for your continued contributions! |
|
Recently, some of my users reported slow or unresponsive widget actions. After investigating, I found the following:
In my testing, when the app is already “warm” (not the first action after a longer idle period):
I have ~50k active users in a project that relies heavily on widgets. I recently released an update using direct execution, and so far the results have been very positive. Users are reporting significantly faster and more responsive widget interactions. Regarding your questions:
To be honest, I struggle to see the benefit of using WorkManager at all. Widget actions are explicitly user-initiated and should execute immediately. Relying on a system like WorkManager, which is subject to Doze mode, OEM restrictions, and scheduling delays, feels counterintuitive for this use case. Am I missing something? |
|
Thanks for the reasoning and the work on this! Agree with also potentially always using this going forward but with this approach I think it leaves the option open as well! |
ABausG
left a comment
ABausG
left a comment
There was a problem hiding this comment.
Overall looks good.
Apart from the formatting failure can you please also add a brief paragraph to the docs to indicate about this option 🙏
2 participants
Description
Adds a new
directexecution mode for Android background callbacks that runs Dart code directly in theBroadcastReceiverusinggoAsync(), bypassing WorkManager entirely.On some devices, WorkManager tasks are delayed between being queued and executed, and on others, they don’t run at all. I’m not sure whether this is an existing issue or something that started happening more recently. #361
HomeWidgetBackgroundIntent.getBroadcast(context, uri, direct = true)Checklist
exampleor documentation.Breaking Change?