fix: guard ensure_channel_ready() against a concurrent-caller race - #3776
fix: guard ensure_channel_ready() against a concurrent-caller race#3776mittalpk wants to merge 1 commit into
Conversation
|
Welcome @mittalpk! It looks like this is your first PR to milvus-io/pymilvus 🎉 |
|
/assign @longjiquan |
|
/assign @longjiquan |
| # recursion limit on a later RPC (interceptor dispatch recurses one | ||
| # frame per interceptor). _reconnect_lock is the same lock reconnect() | ||
| # and close() already use to serialize channel-state mutation. | ||
| async with self._reconnect_lock: |
There was a problem hiding this comment.
pymilvus/client/async_grpc_handler.py line:319
Low ---- The setup now runs while holding _reconnect_lock, so during a connection burst only the first caller's timeout applies to the shared Connect RPC and every other concurrent caller waits on the lock for up to that duration (default 10s) with their own timeout argument ignored on that wait; close() and reconnect() block for the same period because they share this lock. This is the intended trade-off of the fix, but a brief doc note on the method (or an overall deadline around the lock+setup) would make the queued-caller behavior explicit for users that pass small timeouts.
| if self._is_channel_ready: | ||
| return | ||
|
|
||
| # Without the lock, many coroutines can race in here concurrently |
There was a problem hiding this comment.
pymilvus/client/async_grpc_handler.py line:310
Low ---- Question: the linked issue #3030 is still open but carries the wontfix label (added by XuanYang-cn on 2026-01-05), while this PR says "Fixes #3030" and the commit says "Closes #3030". Please confirm with the maintainers that the wontfix decision is being reversed so the issue state and this fix are consistent at merge time.
|
pymilvus/client/async_grpc_handler.py line:304 |
ensure_channel_ready() checked self._is_channel_ready and, if False, awaited _setup_identifier_interceptor_for_channel() before setting it -- with no lock. Every concurrent caller racing in before the first one finished independently awaited the same setup call, which appends a new interceptor to the shared channel's interceptor chain on every call. Enough concurrent callers (a burst of ~490 requests reliably reproduced it) stack enough interceptors to blow Python's default recursion limit on a later RPC, since gRPC's interceptor dispatch recurses one frame per interceptor. Guard the setup with the same _reconnect_lock reconnect() and close() already use to serialize channel-state mutation, with a double-checked _is_channel_ready read so the common already-ready path never takes the lock. Closes milvus-io#3030 Signed-off-by: Praveen Mittal <pkmittal28@gmail.com>
51d9489 to
38dcfec
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mittalpk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4 participants
Fixes #3030.
ensure_channel_ready()checkedself._is_channel_readyand, ifFalse, awaited_setup_identifier_interceptor_for_channel()before setting it — with no lock. Every concurrent caller racing in before the first one finished independently awaited the same setup call, which appends a new interceptor to the shared channel's interceptor chain on every call. Enough concurrent callers (the report's own repro: ~490 concurrent requests) stack enough interceptors to blow Python's default recursion limit on a later RPC, since gRPC's interceptor dispatch recurses one frame per interceptor — exactly matching the reportedRecursionErrorand its concurrency threshold.Guards the setup with the same
_reconnect_lockthatreconnect()andclose()already use to serialize channel-state mutation, with a double-checked_is_channel_readyread so the common already-ready path never takes the lock.Testing
ensure_channel_ready()calls against a mocked setup function and asserts it's only invoked once. Confirmed it fails against unpatched code (20 calls, not 1) viagit stashisolation before confirming it passes here.ruff check/ruff format --check/black --checkall clean.