Skip to content

wslc: Switch port relay AcceptThread from WaitForMultipleObjects to IO completion ports#40042

Open
benhillis wants to merge 7 commits intofeature/wsl-for-appsfrom
user/benhill/fix-port-relay-handle-limit
Open

wslc: Switch port relay AcceptThread from WaitForMultipleObjects to IO completion ports#40042
benhillis wants to merge 7 commits intofeature/wsl-for-appsfrom
user/benhill/fix-port-relay-handle-limit

Conversation

@benhillis
Copy link
Copy Markdown
Member

@benhillis benhillis commented Mar 30, 2026

Replace the WaitForMultipleObjects-based wait loop in relay::MultiHandleWait with an IO completion port (IOCP), removing the MAXIMUM_WAIT_OBJECTS (64) handle limit for all callers (IORelay, container IO, socket accept, etc.).

Switch the port relay AcceptThread from WaitForMultipleObjects to a direct IOCP, removing the 63-port mapping limit.

relay::MultiHandleWait changes (relay.hpp/cpp)

  • Add unique_registered_wait RAII type for RegisterWaitForSingleObject handles
  • Add Register() pure virtual to OverlappedIOHandle, implemented by all subclasses
  • Handles that support IOCP (pipes, sockets) register directly; handles that don't (console handles, events) use a RegisterWaitForSingleObject bridge
  • Convert Run() to use GetQueuedCompletionStatus instead of WaitForMultipleObjects
  • Cancel() posts a key=0 completion to wake Run(); key=0 sets m_cancel on dequeue
  • Fix member order: m_iocp declared before m_handles for correct destruction

Port relay changes (localhost.cpp)

  • Associate listen sockets directly with an IOCP via CreateIoCompletionPort
  • AcceptEx completions go straight to the IOCP — no events or thread pool waits
  • Stop signal via PostQueuedCompletionStatus key=0
  • Remove AcceptEvent from PortRelay (no longer needed)
  • Remove the MAXIMUM_WAIT_OBJECTS port limit check

@benhillis benhillis requested a review from a team as a code owner March 30, 2026 21:19
Copilot AI review requested due to automatic review settings March 30, 2026 21:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the WSLC localhost port relay to use an I/O completion port (IOCP) for AcceptEx completions instead of WaitForMultipleObjects, removing the 64-handle wait limitation and enabling more than 63 concurrent port mappings.

Changes:

  • Replace the accept loop with an IOCP-driven AcceptThread and associate each listen socket with a shared completion port.
  • Remove the relay’s per-port accept event and route both sync/async AcceptEx completions through IOCP.
  • Update Windows tests to validate mapping/unmapping 100 ports instead of enforcing the prior 63-port limit.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/windows/wslrelay/localhost.cpp Switch accept scheduling/completions to IOCP; add stdin MessageReader to coordinate exit + message readiness.
test/windows/WSLCTests.cpp Update port-mapping test to validate >63 mappings (now 100).

Copy link
Copy Markdown
Collaborator

@OneBlue OneBlue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea, but given that the relay code is not our long term plan, I would recommend making this change at the relay::MultiHandleWait level so it will also solve the 64 handles issue in other places in the code (specifically the container IO thread, which could go over the limit if many Logs() / Exec() calls are made)

If we need to, we can then move the relay to use MultiHandleWait and match the rest of the code

@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 7e40403 to 8370b5a Compare April 1, 2026 15:31
Copilot AI review requested due to automatic review settings April 1, 2026 15:34
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 8370b5a to 87b2cd8 Compare April 1, 2026 15:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

@benhillis
Copy link
Copy Markdown
Member Author

I think this is a good idea, but given that the relay code is not our long term plan, I would recommend making this change at the relay::MultiHandleWait level so it will also solve the 64 handles issue in other places in the code (specifically the container IO thread, which could go over the limit if many Logs() / Exec() calls are made)

If we need to, we can then move the relay to use MultiHandleWait and match the rest of the cod

That's a good call, let me look at that.

@benhillis benhillis marked this pull request as draft April 1, 2026 15:46
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 87b2cd8 to 6b9bbbb Compare April 1, 2026 21:26
@benhillis benhillis marked this pull request as ready for review April 1, 2026 21:28
Copilot AI review requested due to automatic review settings April 1, 2026 21:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Copilot AI review requested due to automatic review settings April 3, 2026 15:27
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 3de1c99 to 3fe1174 Compare April 3, 2026 15:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 3fe1174 to 8091b71 Compare April 3, 2026 16:35
Copilot AI review requested due to automatic review settings April 5, 2026 18:33
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 8091b71 to a7ab4d5 Compare April 5, 2026 18:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Copilot AI review requested due to automatic review settings April 6, 2026 01:52
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from b06a0f2 to fdb0b87 Compare April 6, 2026 01:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Ben Hillis added 4 commits April 5, 2026 19:24
…letion ports

Replace the WaitForMultipleObjects-based accept loop in the WSLC port
relay with an IO completion port (IOCP). This removes the
MAXIMUM_WAIT_OBJECTS (64) handle limit, allowing unlimited port
mappings.

Changes:
- Associate each listen socket with a shared IOCP using the PortRelay
  pointer as the completion key.
- Rewrite AcceptThread to use GetQueuedCompletionStatus instead of
  WaitForMultipleObjects. On shutdown, cancel all pending I/O and drain
  completions before returning.
- Remove the AcceptEvent from PortRelay (no longer needed with IOCP).
- Change ScheduleAccept from bool to void since both sync and async
  AcceptEx completions are now delivered through the IOCP.
- Add a MessageReader that reads stdin on a dedicated thread, allowing
  the main loop to WaitForMultipleObjects on the exit event and message
  ready event simultaneously.
- Remove the 63-port limit and update tests to validate 100 port
  mappings.
… AcceptThread

- Add InheritHandle() for m_vmTerminatingEvent so the relay child process
  receives a valid exit event handle via PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
- Close parent pipe handles with .reset() instead of .release() so ReadFile
  in MapRelayPort detects relay process death via pipe EOF.
- Move AcceptThread I/O drain into wil::scope_exit so pending accepts are
  always cancelled even if ScheduleAccept throws during initialization.
- Wrap initial ScheduleAccept loop in try/catch to prevent a single port
  failure from killing the entire accept thread.
- Track accept thread liveness with std::atomic<bool> to avoid posting
  stale IOCP exit signals when the thread has already exited.
- Make PortRelay destructor cancel pending I/O instead of __fastfail.
- Add <atomic> to precomp.h.
…IO completion ports

Replace the WaitForMultipleObjects-based wait loop in relay::MultiHandleWait
with an IO completion port (IOCP), removing the MAXIMUM_WAIT_OBJECTS (64)
handle limit for all callers.

Changes to relay::MultiHandleWait (relay.hpp/cpp):
- Add unique_registered_wait RAII type for RegisterWaitForSingleObject handles
- Add Register() pure virtual to OverlappedIOHandle, implemented by all subclasses
- Handles that support IOCP (pipes, sockets) register directly; handles that
  don't (console handles, events) use a RegisterWaitForSingleObject bridge
- Convert Run() to use GetQueuedCompletionStatus instead of WaitForMultipleObjects
- Cancel() posts a key=0 completion to wake Run(); key=0 sets m_cancel on dequeue
- Fix member order: m_iocp declared before m_handles for correct destruction

Changes to port relay (localhost.cpp):
- Replace AcceptThread's WaitForMultipleObjects with a direct IOCP: associate
  listen sockets via CreateIoCompletionPort, AcceptEx completions go straight
  to the IOCP, stop signal via PostQueuedCompletionStatus key=0
- Remove the MAXIMUM_WAIT_OBJECTS port limit check
- Remove AcceptEvent from PortRelay (no longer needed with direct IOCP)
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from fdb0b87 to a9ecbec Compare April 6, 2026 04:06
Copilot AI review requested due to automatic review settings April 6, 2026 05:12
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from a9ecbec to 677ed96 Compare April 6, 2026 05:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

…onModes

Two fixes:

1. relay.cpp: Call SetFileCompletionNotificationModes BEFORE
   CreateIoCompletionPort. If FILE_SKIP_COMPLETION_PORT_ON_SUCCESS is
   not supported for a handle type (e.g. certain socket types), fall
   back to event-based mode entirely. Previously the handle was
   associated with the IOCP without skip-on-success, causing
   synchronous completions to both be processed inline by Schedule()
   and queued to the IOCP — a double-processing race that manifests
   as ERROR_NO_DATA (232) in ContainerLogs/ContainerAttach tests.

2. localhost.cpp: THROW_IF_WIN32_BOOL_FALSE on
   SetFileCompletionNotificationModes in the port relay AcceptThread.

Co-authored-by: Copilot <[email protected]>
@benhillis benhillis force-pushed the user/benhill/fix-port-relay-handle-limit branch from 677ed96 to c712ebf Compare April 6, 2026 15:27
ReadHandle, SingleAcceptHandle, and WriteHandle destructors skipped
waiting for CancelIoEx completion when RegisteredWithIocp was true.
Since OVERLAPPED is a member, this creates a use-after-free race:
the kernel may still reference the OVERLAPPED after the destructor
frees it. Always wait for the cancellation to complete regardless
of IOCP registration. The stale IOCP completion is harmlessly
discarded by MultiHandleWait::Run() (handle already removed from
m_handles).

Co-authored-by: Copilot <[email protected]>
Copilot AI review requested due to automatic review settings April 6, 2026 17:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

…iation

SetFileCompletionNotificationModes must be called before
CreateIoCompletionPort to prevent a race where a synchronous
AcceptEx completion could queue an IOCP packet before the skip
mode is enabled, causing double-processing.

Co-authored-by: Copilot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants