fix: return background task from consume_and_break_on_interrupt to prevent GC#775
fix: return background task from consume_and_break_on_interrupt to prevent GC#775kevinlu310 wants to merge 3 commits intoa2aproject:mainfrom
Conversation
…event GC ResultAggregator.consume_and_break_on_interrupt creates a background asyncio.Task to continue consuming events after an interruption (non-blocking or auth_required), but discards the task reference. On Python 3.12+ the event loop only holds weak references to tasks, so the garbage collector can silently collect the task before it completes — dropping remaining events (completed/failed status) and push notification callbacks. Return the background task as a third tuple element so callers can hold a strong reference. DefaultRequestHandler.on_message_send now tracks it via _track_background_task(), the same mechanism already used for other background work.
Summary of ChangesHello, 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 addresses a critical issue in Python 3.12+ where background Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request addresses a critical garbage collection issue with asyncio.Task on Python 3.12+ by ensuring a strong reference is held for background tasks. The approach of returning the background task from consume_and_break_on_interrupt and tracking it in DefaultRequestHandler is correct and well-implemented. The changes are clean, include necessary type hint updates and excellent docstring explanations. The tests have been thoroughly updated to reflect the new return signature. I've found one minor redundancy in a test mock setup that could be cleaned up.
|
|
||
| queue = await self._queue_manager.create_or_tap(task_id) | ||
| result_aggregator = ResultAggregator(task_manager) | ||
| # TODO: to manage the non-blocking flows. |
There was a problem hiding this comment.
I believe that this comment should be also removed now
Description
ResultAggregator.consume_and_break_on_interrupt creates a background asyncio.Task to continue consuming events after an interruption (non-blocking or auth_required), but discards the task reference. On Python 3.12+ the event loop only holds weak references to tasks, so the garbage collector can silently collect the task before it completes — dropping remaining events (completed/failed status) and push notification callbacks.
Return the background task as a third tuple element so callers can hold a strong reference. DefaultRequestHandler.on_message_send now tracks it via _track_background_task(), the same mechanism already used for other background work.
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.bash scripts/format.shfrom the repository root to format)Fixes #774 🦕