Pigeon Task Queues - #117
Conversation
![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 transitions calendar operations to a serial background thread using Pigeon's task queue feature, removing coroutine scopes in favor of synchronous execution. However, the introduction of synchronous permission checks using CountDownLatch in PermissionHandler.kt poses a severe risk of deadlocks and thread starvation on the serial background queue. Additionally, several methods in CalendarImplem.kt catch specific FlutterError exceptions within generic Exception blocks, resulting in lost error codes (such as NOT_FOUND) being wrapped in GENERIC_ERROR.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fun requestReadPermissionSync(): Boolean { | ||
| var result = false | ||
| val latch = CountDownLatch(1) | ||
|
|
||
| requestReadPermission { granted -> | ||
| result = granted | ||
| latch.countDown() | ||
| } | ||
|
|
||
| latch.await() | ||
| return result | ||
| } | ||
|
|
||
| fun requestWritePermissionSync(): Boolean { | ||
| var result = false | ||
| val latch = CountDownLatch(1) | ||
|
|
||
| requestWritePermission { granted -> | ||
| result = granted | ||
| latch.countDown() | ||
| } | ||
|
|
||
| latch.await() | ||
| return result | ||
| } |
There was a problem hiding this comment.
Thread Blocking & Deadlock Risk in Synchronous Permission Requests
Using CountDownLatch to block the calling thread while waiting for an asynchronous OS permission dialog is highly discouraged and introduces several severe risks:
- Deadlock Risk: If
requestReadPermissionSyncorrequestWritePermissionSyncis ever called on the main thread, the app will deadlock. The main thread will be blocked atlatch.await(), preventingactivity.runOnUiThreadfrom executing the permission request on the main thread's message loop. - Serial Queue Starvation: Since the Pigeon API is configured to use
TaskQueueType.serialBackgroundThread, all calendar operations share a single background thread. Blocking this thread while waiting for user interaction (which can take seconds or minutes) will completely freeze all other calendar operations queued behind it. - Thread Leaks / Indefinite Blocking: If the activity is destroyed, recreated, or the permission request is cancelled by the OS without triggering the callback, the
CountDownLatchwill never be counted down, blocking the background thread indefinitely.
Recommendation
Keep the permission handling asynchronous. Since Pigeon's @async methods already expect a callback (callback: (Result<T>) -> Unit), you can perform the permission check asynchronously and only invoke the callback once the permission is granted or denied, without blocking any threads.
|
|
||
| } catch (e: Exception) { | ||
| callback( | ||
| Result.failure( | ||
| FlutterError( | ||
| code = "GENERIC_ERROR", | ||
| message = e.message, | ||
| details = e.cause | ||
| ) | ||
| ) | ||
| } catch (e: Exception) { | ||
| callback( | ||
| Result.failure( | ||
| FlutterError( | ||
| code = "GENERIC_ERROR", | ||
| message = e.message, | ||
| details = e.cause | ||
| ) | ||
| } | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Lost Error Code (NOT_FOUND wrapped in GENERIC_ERROR)
When retrieveEvent(eventId) throws a FlutterError (e.g., with code "NOT_FOUND" if the event does not exist), it is caught by the generic catch (e: Exception) block and wrapped in a new FlutterError with code "GENERIC_ERROR". This causes the specific "NOT_FOUND" error code to be lost.
To preserve the specific error code, add a catch block for FlutterError before the generic Exception catch block, similar to how it is done in createEvent and deleteEvent.
} catch (e: FlutterError) {
callback(Result.failure(e))
} catch (e: Exception) {
callback(
Result.failure(
FlutterError(
code = "GENERIC_ERROR",
message = e.message,
details = e.cause
)
)
)
}| } catch (e: Exception) { | ||
| callback( | ||
| Result.failure( | ||
| FlutterError( | ||
| code = "GENERIC_ERROR", | ||
| message = e.message, | ||
| details = e.cause | ||
| ) | ||
| } | ||
| } | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Lost Error Code (NOT_FOUND wrapped in GENERIC_ERROR)
When retrieveEvent(eventId) throws a FlutterError (e.g., with code "NOT_FOUND" if the event does not exist), it is caught by the generic catch (e: Exception) block and wrapped in a new FlutterError with code "GENERIC_ERROR". This causes the specific "NOT_FOUND" error code to be lost.
To preserve the specific error code, add a catch block for FlutterError before the generic Exception catch block, similar to how it is done in createEvent and deleteEvent.
} catch (e: FlutterError) {
callback(Result.failure(e))
} catch (e: Exception) {
callback(
Result.failure(
FlutterError(
code = "GENERIC_ERROR",
message = e.message,
details = e.cause
)
)
)
}| } catch (e: Exception) { | ||
| callback( | ||
| Result.failure( | ||
| FlutterError( | ||
| code = "GENERIC_ERROR", | ||
| message = e.message, | ||
| details = e.cause | ||
| ) | ||
| } | ||
| } | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Lost Error Code (NOT_FOUND wrapped in GENERIC_ERROR)
When retrieveEvent(eventId) throws a FlutterError (e.g., with code "NOT_FOUND" if the event does not exist), it is caught by the generic catch (e: Exception) block and wrapped in a new FlutterError with code "GENERIC_ERROR". This causes the specific "NOT_FOUND" error code to be lost.
To preserve the specific error code, add a catch block for FlutterError before the generic Exception catch block, similar to how it is done in createEvent and deleteEvent.
} catch (e: FlutterError) {
callback(Result.failure(e))
} catch (e: Exception) {
callback(
Result.failure(
FlutterError(
code = "GENERIC_ERROR",
message = e.message,
details = e.cause
)
)
)
}| } catch (e: Exception) { | ||
| callback( | ||
| Result.failure( | ||
| FlutterError( | ||
| code = "GENERIC_ERROR", | ||
| message = e.message, | ||
| details = e.cause | ||
| ) | ||
| } | ||
| } | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Lost Error Code (NOT_FOUND wrapped in GENERIC_ERROR)
When retrieveEvent(eventId) throws a FlutterError (e.g., with code "NOT_FOUND" if the event does not exist), it is caught by the generic catch (e: Exception) block and wrapped in a new FlutterError with code "GENERIC_ERROR". This causes the specific "NOT_FOUND" error code to be lost.
To preserve the specific error code, add a catch block for FlutterError before the generic Exception catch block, similar to how it is done in createEvent and deleteEvent.
} catch (e: FlutterError) {
callback(Result.failure(e))
} catch (e: Exception) {
callback(
Result.failure(
FlutterError(
code = "GENERIC_ERROR",
message = e.message,
details = e.cause
)
)
)
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
============================================
+ Coverage 88.09% 93.57% +5.48%
============================================
Files 23 8 -15
Lines 4207 2818 -1389
Branches 112 0 -112
============================================
- Hits 3706 2637 -1069
+ Misses 422 181 -241
+ Partials 79 0 -79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
No description provided.