-
Notifications
You must be signed in to change notification settings - Fork 461
fix: guard ensure_channel_ready() against a concurrent-caller race #3776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -302,8 +302,24 @@ def get_server_type(self): | |
| return get_server_type(self.server_address.split(":")[0]) | ||
|
|
||
| async def ensure_channel_ready(self, timeout: Optional[float] = None): | ||
| try: | ||
| if not self._is_channel_ready: | ||
| # Fast path: avoid the lock once a real request has already set this, | ||
| # which is the common case for every call after the first. | ||
| if self._is_channel_ready: | ||
| return | ||
|
|
||
| # Without the lock, many coroutines can race in here concurrently | ||
| # while _is_channel_ready is still False: each would independently | ||
| # await _setup_identifier_interceptor_for_channel(), which appends | ||
| # its own interceptor to the shared channel's interceptor chain on | ||
| # every call. Enough concurrent callers before the first one sets | ||
| # _is_channel_ready stacks enough interceptors to blow Python's | ||
| # 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pymilvus/client/async_grpc_handler.py line:319 |
||
| if self._is_channel_ready: | ||
| return | ||
| try: | ||
| wait_timeout = timeout if timeout is not None else 10 | ||
| ( | ||
| self._async_identifier_interceptor, | ||
|
|
@@ -317,11 +333,11 @@ async def ensure_channel_ready(self, timeout: Optional[float] = None): | |
| ) | ||
|
|
||
| self._is_channel_ready = True | ||
| except (grpc.FutureTimeoutError, asyncio.TimeoutError, grpc.RpcError) as e: | ||
| raise MilvusException( | ||
| code=Status.CONNECT_FAILED, | ||
| message=f"Fail connecting to server on {self._address}, illegal connection params or server unavailable", | ||
| ) from e | ||
| except (grpc.FutureTimeoutError, asyncio.TimeoutError, grpc.RpcError) as e: | ||
| raise MilvusException( | ||
| code=Status.CONNECT_FAILED, | ||
| message=f"Fail connecting to server on {self._address}, illegal connection params or server unavailable", | ||
| ) from e | ||
|
|
||
| async def _register_identifier(self, stub: Any, user: str, timeout: float = 10) -> int: | ||
| host = socket.gethostname() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pymilvus/client/async_grpc_handler.py line:310
Low ---- Question: the linked issue #3030 is still open but carries the
wontfixlabel (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.