fix: fix error handling for gRPC and SSE streaming#879
fix: fix error handling for gRPC and SSE streaming#879guglielmo-san merged 13 commits into1.0-devfrom
Conversation
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 refactors the gRPC request handling mechanism to centralize error management and context building. By introducing generic helper methods for unary and streaming calls, it standardizes how errors are caught and processed across all gRPC endpoints. This change improves code maintainability and ensures consistent error responses, while also enhancing test coverage for various error conditions. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request effectively refactors the gRPC handlers to centralize error handling and context management, improving code organization and reducing duplication. The changes in src/a2a/compat/v0_3/grpc_handler.py correctly move validation decorators to nested functions, aligning with the new error handling strategy. The addition of new fixtures and integration tests in tests/integration/test_client_server_integration.py demonstrates a commitment to robust testing, particularly for the new error handling and compatibility features. The overall direction of the changes is positive for maintainability and reliability.
852df79 to
f0acbd8
Compare
f0acbd8 to
7236d8c
Compare
2f3b48f to
a84f89e
Compare
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/client/transports/grpc.py | 88.46% | 89.23% | 🟢 +0.77% |
| src/a2a/client/transports/http_helpers.py | 95.65% | 94.64% | 🔴 -1.01% |
| src/a2a/client/transports/jsonrpc.py | 83.92% | 86.71% | 🟢 +2.80% |
| src/a2a/compat/v0_3/grpc_handler.py | 95.52% | 97.01% | 🟢 +1.49% |
| src/a2a/compat/v0_3/grpc_transport.py | 41.84% | 59.57% | 🟢 +17.73% |
| src/a2a/server/request_handlers/grpc_handler.py | 81.55% | 94.48% | 🟢 +12.93% |
| src/a2a/utils/helpers.py | 97.42% | 97.19% | 🔴 -0.23% |
| Total | 90.77% | 91.45% | 🟢 +0.68% |
Generated by coverage-comment.yml
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors error handling for gRPC and SSE streaming. The gRPC handler refactoring significantly improves code clarity and maintainability by centralizing error handling. The SSE error handling improvements in http_helpers.py are also a good addition for robustness.
However, there is a critical issue introduced by replacing @validate_async_generator with @validate on async generator methods. The current @validate decorator is not designed to handle async generators and will cause a TypeError at runtime. This affects gRPC, REST, and JSON-RPC streaming handlers. I've left a detailed comment on this issue. Additionally, the new integration tests for this feature are incomplete as they only test the failure path, which masks this bug.
Reproduced in
test_client_server_integration.py.gRPC
validate_async_generatordecorator was applied on top of the method above A2A error handling. Compat handler was already refactored in a way which made it possible to apply it on a nested function. It was done there and v1 handler was refactored in the same way.SSE streaming
Iterator wrapped into
validate_async_generatoris assigned toEventSourceResponseand is returned from the method, so when it throwsrest_stream_error_handlerhas no effect on it.a2a-python/src/a2a/server/apps/rest/rest_adapter.py
Lines 155 to 163 in 4630efd
Instead of throwing on the first iteration, throw on the method invocation itself to avoid more sophisticated error handling (i.e. reading one item to trigger error) by removing separate handling for async generator.
Client-level handling is also updated to properly handle non-200 status code for streaming and non-streaming response in case of JSON-RPC error.