[Bug][Update][Upstream] Fix AscendPDBacked init for v0.4.5#265
[Bug][Update][Upstream] Fix AscendPDBacked init for v0.4.5#265marcobarlo wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request overrides the _ensure_peer_connection method in sender_mixin.py to establish a decoder peer connection over HCCL directly, bypassing the upstream implementation that relies on an unavailable lock. Feedback highlights a potential race condition where concurrent calls to this method could result in duplicate ZMQ socket creation and file descriptor leaks. It is recommended to wrap the initialization logic in a lock to ensure thread safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if receiver_id in self.initialized_peers: | ||
| return | ||
|
|
||
| receiver_init_url = f"{receiver_host}:{receiver_init_port}" | ||
| receiver_mem_alloc_url = f"{receiver_host}:{receiver_alloc_port}" | ||
|
|
||
| self.transfer_channel.lazy_init_peer_connection( | ||
| local_id=self.local_id, | ||
| peer_id=receiver_id, | ||
| peer_init_url=receiver_init_url, | ||
| ) | ||
|
|
||
| mem_alloc_socket = get_zmq_socket( | ||
| self.zmq_context, | ||
| receiver_mem_alloc_url, | ||
| "tcp", | ||
| zmq.REQ, | ||
| "connect", | ||
| ) | ||
| self.mem_alloc_sockets[receiver_id] = mem_alloc_socket | ||
| self.initialized_peers.add(receiver_id) |
There was a problem hiding this comment.
The _ensure_peer_connection method can be called concurrently by multiple threads (e.g., during concurrent request processing). Since checking and updating self.initialized_peers and self.mem_alloc_sockets is not atomic, a race condition exists where multiple threads could concurrently attempt to initialize the same peer connection. This can lead to duplicate ZMQ socket creation, overwriting existing sockets in self.mem_alloc_sockets, and leaking file descriptors.
To prevent this, wrap the connection initialization logic in a lock. We can reuse self._peer_alloc_backoff_lock which is already initialized and available on the instance.
with self._peer_alloc_backoff_lock:
if receiver_id in self.initialized_peers:
return
receiver_init_url = f"{receiver_host}:{receiver_init_port}"
receiver_mem_alloc_url = f"{receiver_host}:{receiver_alloc_port}"
self.transfer_channel.lazy_init_peer_connection(
local_id=self.local_id,
peer_id=receiver_id,
peer_init_url=receiver_init_url,
)
mem_alloc_socket = get_zmq_socket(
self.zmq_context,
receiver_mem_alloc_url,
"tcp",
zmq.REQ,
"connect",
)
self.mem_alloc_sockets[receiver_id] = mem_alloc_socket
self.initialized_peers.add(receiver_id)There was a problem hiding this comment.
@matthewygf I think this is not a valid comment as the peer_id is unique on sender side for any given rank of a D instance. Am I right?
Summary
This PR fixes issue #264 .
AttributeError: 'AscendPDBackend' object has no attribute '_nixl_agent_lock'.super()._ensure_peer_connection(...)with an HCCL-nativepeer init (lazy channel connect + alloc REQ socket). Do not call
upstream NIXL
_ensure_peer_connection.AscendPDSenderMixin._ensure_peer_connectiononly(
lmcache_ascend/v1/storage_backend/pd/sender_mixin.py).Why
7f60057c(#2972, first in v0.4.5) added_nixl_agent_lockfor the async NIXL worker thread.AscendPDBackend.__init__never callsPDBackend.__init__, sothat lock is never created. HCCL already serializes via
transfer_channel._state_lock; Ascend does not need_nixl_agent_lock.Test plan
docs/fix_missing_nixl_agent_lock.diff)._nixl_agent_lock; peer connect succeeds.