[Python] Added registered methods support in AsyncIO stack #41796
[Python] Added registered methods support in AsyncIO stack #41796Zgoda91 wants to merge 23 commits into
Conversation
eb9a7bc to
6a188ca
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds client-side registered method support for the gRPC Python AsyncIO stack, primarily for observability purposes. It also enables server-side observability for AsyncIO and extends the test suite to cover these new features. The changes are well-structured, introducing necessary modifications across the Cython and Python layers, and include comprehensive tests for both registered and unregistered methods. My feedback includes a suggestion to refactor duplicated code for better maintainability and a recommendation to reduce a long sleep in the tests to improve execution speed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for registered methods in the Python AsyncIO stack for observability. However, it introduces two significant Denial of Service (DoS) vulnerabilities related to NULL pointer dereferences: one in observability hooks where call tracers are dereferenced without NULL checks, and another regression in the AsyncIO server stack causing early unreffing of gRPC calls. Addressing these is crucial for server stability. Additionally, there are opportunities for minor refactoring to improve code maintainability by reducing duplication.
491ae84 to
4918c90
Compare
f32034c to
0756a51
Compare
0756a51 to
e34b3ef
Compare
asheshvidyut
left a comment
There was a problem hiding this comment.
Will it be possible to run Pyright over the changed files, to see if the Type Annotations are correct?
We are planning to add Pyright in CI soon - #41210
|
Will review other files after we have consensus on the comments added above. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements support for registered methods in gRPC AsyncIO, enhancing performance for both client-side calls and server-side request handling. The implementation includes updates to the Cython layer for registered call APIs, a new method resolver for efficient dispatching, and comprehensive test coverage for interceptors and observability. Review feedback highlights a potential race condition in the registration of call handles and the risk of a KeyError when retrieving registered methods on the server, recommending the use of synchronization primitives and safer dictionary access.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements support for registered method handlers in gRPC Asyncio Python, updating both client and server call paths to handle registered call handles and adding corresponding unit and observability tests. The review feedback highlights a critical bug in the server main loop where a failed request call future could permanently stop the server from listening to that method, and suggests making the test helper assert_eventually asynchronous to avoid blocking the event loop thread with time.sleep.
| try: | ||
| rpc_state = completed.result() | ||
| except: | ||
| # This slot failed because server is shutting down, so | ||
| # skip it (no limiter change) but keep serving the other | ||
| # calls in this batch | ||
| continue |
There was a problem hiding this comment.
If completed.result() raises an exception while the server is still running (self._status == AIO_SERVER_STATUS_RUNNING), the except block will execute continue and skip the recreation of the request call future for this slot. This means that if a call request fails once (e.g., due to a transient error or internal error), the server will permanently stop listening for new calls on that specific registered method (or stop accepting generic calls entirely if it was the generic slot).
We should ensure that the request call future is recreated if the server is still running.
try:
rpc_state = completed.result()
except:
# This slot failed because server is shutting down, so
# skip it (no limiter change) but keep serving the other
# calls in this batch
if self._status == AIO_SERVER_STATUS_RUNNING:
new_fut = self._make_request_call_future(method_bytes)
pending_futures[new_fut] = method_bytes
continue
Fixing #39800 and #39061
Work done:
Should be merged after #41573 as this code is an extension for mentioned PR.