[PAL] fix p2p socket destruction issue#502
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses noisy warnings and teardown hazards in the P2P engine by adding an explicit accept-cancellation mechanism so listener sockets aren’t freed/closed while accept loops (bootstrap + IB) may still be running.
Changes:
- Introduces abort-flag awareness in the socket layer to treat abort-triggered accept failures (e.g., EBADF/EINVAL during teardown) as expected cancellation.
- Adds
flagcxP2pEngineStopAccept()flow to signal shutdown, abort/wake accept loops, and join the RPC accept thread before final cleanup. - Extends the IB RC P2P adaptor with an abort-listen path and wires abort flags through bootstrap/IB accept codepaths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| flagcx/service/bootstrap.cc | Passes abort flag through bootstrap accept socket init and adds accept failure cleanup. |
| flagcx/core/socket.cc | Adds abort-flag helper and suppresses warnings for abort-triggered accept failures; propagates abort as cancellation. |
| flagcx/core/flagcx_p2p.cc | Adds engine-level stop-accept signaling, abort flag plumbing, listener abort calls, and thread join ordering. |
| flagcx/adaptor/net/ibrc_p2p_adaptor.cc | Adds abort flag to listen comm, integrates abort into accept/ready loop, and exposes abort-listen helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1869
to
+1873
| engine->stopAccept.store(true, std::memory_order_release); | ||
| engine->stopNotif = true; | ||
| engine->stopRpcServer.store(true, std::memory_order_release); | ||
| __atomic_store_n(&engine->acceptAbortFlag, 1, __ATOMIC_RELEASE); | ||
|
|
MC952-arch
reviewed
Jun 17, 2026
| size_t ipAddrBufLen, int *remoteGpuIdx) { | ||
| if (engine == NULL || ipAddrBuf == NULL || remoteGpuIdx == NULL) | ||
| return NULL; | ||
| if (engine->stopAccept.load(std::memory_order_acquire)) |
Collaborator
There was a problem hiding this comment.
either use std::atomic::load/store or __atomic_load/store_n
Collaborator
Author
There was a problem hiding this comment.
engine->stopAccept is of type std::atomic<bool>, so the load operation here is using std::atomic::load
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Category
PAL
PR Types
Bug Fixes
PR Description
This PR fixes the socket destruction problem:
Root Cause
The P2P engine shutdown path closed and freed bootstrap/IB listen sockets while an accept loop could still be blocked on them. During teardown, accept() could observe a closed or
invalid listener fd and return EINVAL/EBADF, which surfaced as noisy socket warnings even though data transfer had already completed successfully.
Solution
This PR adds an explicit accept-cancellation path for the FlagCX P2P engine. flagcxP2pEngineStopAccept() now signals accept shutdown, wakes bootstrap and IB accept loops via abort-
enabled listen sockets, and joins the internal RPC accept thread before final resource cleanup. Listener resources are only freed during engine destruction after accept users have
exited. The socket layer also treats abort-triggered accept failures as expected cancellation, avoiding false warning logs during normal teardown.