Use RegisterWaitForSingleObject() in MultiHandleIOWait#40658
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates wsl::windows::common::io::MultiHandleWait to use RegisterWaitForSingleObject() callbacks instead of WaitForMultipleObjects(), removing the 64-handle waiting limitation and validating the behavior with new unit/e2e tests.
Changes:
- Reworked
MultiHandleWait::Run()to register per-handle waits and use a shared notification event when any handle signals. - Added a unit test that waits on 100 events to validate behavior above
MAXIMUM_WAIT_OBJECTS. - Added an end-to-end WSLC test that starts 100 concurrent
docker logs -ffollowers to exercise the new wait path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/windows/common/HandleIO.h |
Refactors MultiHandleWait storage to per-handle Entry objects and introduces signaling queue/event members. |
src/windows/common/HandleIO.cpp |
Implements the new wait strategy using RegisterWaitForSingleObject() and a registered-wait RAII wrapper. |
test/windows/UnitTests.cpp |
Adds coverage ensuring MultiHandleWait can process >64 handles in both pre-signaled and live-signaled scenarios. |
test/windows/WSLCTests.cpp |
Adds a stress-style test for many concurrent log followers to validate real-world usage beyond 64 waits. |
| MultiHandleWait& MultiHandleWait::operator=(MultiHandleWait&& other) noexcept | ||
| { | ||
| if (this != &other) | ||
| { | ||
| m_handles = std::move(other.m_handles); | ||
| m_handleSignaledEvent = std::move(other.m_handleSignaledEvent); | ||
| m_cancel = other.m_cancel; | ||
|
|
||
| // N.B. moving a MultiHandleWait() while running is not supported | ||
| WI_ASSERT(m_signaledHandles.empty()); | ||
| } |
There was a problem hiding this comment.
This feels like it is still an issue with an assert. Run should either permanently make the object unmovable or mark it unmovable until Run returns.
There was a problem hiding this comment.
I considered doing something like this, but it felt overkill to add synchronization logic for this, since this would only be an issue if the object was moved while another thread is running it, which would be undefined behavior either way.
Also note that the diff of this comment is outdated. The latest iteration updates the self field when moved
benhillis
left a comment
benhillis
left a comment
There was a problem hiding this comment.
This looks good, I think just that one copilot comment about the move contrustor.
Summary of the Pull Request
This change switches the IO scheduling logic to use
RegisterWaitForSingleObject()instead ofWaitForMultipleObject(), which allows us to wait for more than 64 handles at the same time.PR Checklist
Detailed Description of the Pull Request / Additional comments
I also experimented with IO completion ports, but the change became too complex (see #40569).
This solution is self contained and works well with our current design
Validation Steps Performed