fix: Client hangs when implementing AgentExecutor and awaiting twice in execute method
#379
Conversation
…r and awaiting twice in execute method
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @meoow113, 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 implements a crucial bug fix related to client hanging issues in the event processing pipeline. The solution involves enhancing the EventQueue and EventConsumer components to ensure that all pending events are explicitly cleared before an event queue is closed. This prevents potential deadlocks or resource contention that could lead to the client becoming unresponsive, particularly in scenarios involving complex agent execution flows.
Highlights
- Bug Fix: Client Hanging Issue: This pull request directly addresses a bug where the client would hang, particularly when using
AgentExecutorand encountering a doubleawaitin anexecutemethod. The core of the fix lies in ensuring proper cleanup of event queues. - New Event Queue Management Method: Introduced a new asynchronous method,
clear_events, within theEventQueueclass. This method is designed to efficiently remove all pending events from the current queue and, optionally, from any associated child queues, preventing unprocessed events from causing issues. - Enhanced Event Consumer Cleanup: Modified the
EventConsumerto utilize the newly addedclear_eventsmethod. Specifically, when a final event is detected and the consumer is preparing to close its queue, it now explicitly clears any remaining events, ensuring a clean shutdown and resolving the hanging behavior.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a clear_events method to the EventQueue to address a client hanging issue. The clear_events method clears pending events before closing the queue, preventing deadlocks. A potential concurrency issue was identified where the lock is held for an extended period while clearing child queues. A suggestion has been provided to refactor this for improved performance and safety by releasing the lock before awaiting the child tasks.
holtskinner
changed the title
AgentExecutor and awaiting twice in execute method
holtskinner
left a comment
holtskinner
left a comment
There was a problem hiding this comment.
Can you please add more in your PR description for what this does?
ok, i have updated my PR description |
… forced queue shutdown - Add immediate parameter to EventQueue.close() method to allow immediate queue closure by discarding all pending events - Update EventConsumer.consume_all() to use close(immediate=True) when encountering the first final event to prevent blocking on subsequent final events - This resolves the issue where multiple final events in queue would cause the consumer to block after processing the first final event The immediate parameter provides a way to force close the queue without waiting for all events to be processed, which is useful in scenarios where the consumer needs to exit immediately upon encountering a final event, regardless of remaining events in the queue.
Updated and improved code comments/docstrings. Updated the format
|
I have explained this in other replay |
I have explained this in other replay |
|
Overall, this looks good. Can we add unit tests to exercise the new patterns/behaviors? |
…r and clear_events method - Add tests for close(immediate=True) behavior and child queue propagation - Add tests for clear_events() with various scenarios (empty, closed, with children) - Add tests for concurrent execution and exception handling in clear_events - Remove redundant test_close_with_immediate_false as existing tests already cover this path - Fix linter errors in Python version-specific test mocks Ensures proper test coverage for the new queue shutdown and cleanup functionality.
…3/a2a-python into fix-bug#367-client_hangs
… loop - Replace per-iteration try/except with a single try/except around a while True loop - Exit on asyncio.QueueEmpty to stop draining when the queue is empty - Reduces exception overhead and satisfies Ruff PERF203 without changing behavior
…ility - Move try/except outside the loop to eliminate per-iteration exception overhead (fixes Ruff PERF203) - Keep lock scope minimal: drain parent queue under lock, release before awaiting children - Use asyncio.gather for concurrent child queue clearing with safe exception handling - Handle Python 3.13 asyncio.QueueShutDown in clear_events via a version-safe except clause (fallback to QueueEmpty on <=3.12) No public API changes; behavior is preserved on older Python versions.
…y compatibility - Replace getattr-based exception handling with explicit version check - Use type(e).__name__ comparison to identify QueueShutDown in Python 3.13+ - Re-raise unexpected exceptions to maintain proper error propagation - Fixes mypy error: Exception type must be derived from BaseException
🤖 I have created a release *beep* *boop* --- ## [0.3.2](v0.3.1...v0.3.2) (2025-08-20) ### Bug Fixes * Add missing mime_type and name in proto conversion utils ([#408](#408)) ([72b2ee7](72b2ee7)) * Add name field to FilePart protobuf message ([#403](#403)) ([1dbe33d](1dbe33d)) * Client hangs when implementing `AgentExecutor` and `await`ing twice in execute method ([#379](#379)) ([c147a83](c147a83)) * **grpc:** Update `CreateTaskPushNotificationConfig` endpoint to `/v1/{parent=tasks/*/pushNotificationConfigs}` ([#415](#415)) ([73dddc3](73dddc3)) * make `event_consumer` tolerant to closed queues on py3.13 ([#407](#407)) ([a371461](a371461)) * non-blocking `send_message` server handler not invoke push notification ([#394](#394)) ([db82a65](db82a65)) * **proto:** Add `icon_url` to `a2a.proto` ([#416](#416)) ([00703e3](00703e3)) * **spec:** Suggest Unique Identifier fields to be UUID ([#405](#405)) ([da14cea](da14cea)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
3 participants
Fixes #367