Repository navigation
Fix relay loop to track closed states independently for left and righ… - #41342
Open
Eamon (Eamon2009) wants to merge 12 commits into
Open
Eamon (Eamon2009) wants to merge 12 commits into
Eamon (Eamon2009) wants to merge 12 commits into
Conversation
…y half-close and ioctl interface name handling Change || to && in relay loop so both directions drain before exit Null-terminate interface name after memcpy from ifreq
This commit updates the localhost.cpp file by adding new functionality for monitoring TCP sockets, managing relays, and handling socket communication. It also includes error handling improvements and updates to the port tracking mechanism.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows relay implementation to correctly support half-closed bidirectional connections by tracking each side’s read-closure independently, avoiding premature loop termination when only one direction has closed.
Changes:
- Introduces
leftReadClosed/rightReadClosedto track EOF/closure per side independently of handle nullness. - Updates the relay loop termination condition to exit only when both read directions are closed.
- Stops nulling
LeftHandle/RightHandleon EOF and instead marks the corresponding read side closed, preserving the ability to continue relaying data in the opposite direction.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Eamon (Eamon2009)
marked this pull request as ready for review
August 13, 2026 10:16
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Eamon (Eamon2009)
marked this pull request as draft
August 13, 2026 10:26
Eamon (Eamon2009)
marked this pull request as ready for review
August 13, 2026 10:32
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/windows/common/relay.cpp:321
- Please add a regression test for this new half-close exit condition. No test currently invokes
BidirectionalRelay; a socket-pair test should close one peer's send direction, verify buffered traffic still relays in the opposite direction, then close the second direction and verify the relay thread exits. Without that coverage, the original premature-exit/hang behavior can be reintroduced unnoticed.
if (leftReadClosed && rightReadClosed)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linux/init/localhost.cpp:327
- The disconnect regression is not covered. The existing
NatLocalhostRelaytest only verifies a normal host-to-guest transfer (test/windows/NetworkTests.cpp:2578) and never half-closes a peer or checks that the relay terminates without spinning. Add a regression case that triggersPOLLHUP/one-sided EOF, verifies buffered traffic in the opposite direction is preserved, and confirms the worker exits after both reads close.
if (pollDescriptors[Index].revents & (POLLIN | POLLERR | POLLHUP))
Eamon (Eamon2009)
marked this pull request as draft
August 14, 2026 16:27
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linux/init/localhost.cpp:317
- Add a regression test that half-closes one relay endpoint, verifies buffered/reverse-direction data still flows, and confirms the worker exits after the second endpoint closes. The existing
ValidateLocalhostRelayTraffictest only checks a single host-to-guest transfer and does not exercise the disconnect path fixed here, so both the hang and early-exit behaviors can regress unnoticed.
if ((pollDescriptors[0].fd == -1) && (pollDescriptors[1].fd == -1))
Eamon (Eamon2009)
marked this pull request as ready for review
August 14, 2026 17:05
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| auto& ifRequest = *reinterpret_cast<ifreq*>(ifreqMemory->data()); | ||
| memcpy(request.InterfaceName, ifRequest.ifr_ifrn.ifrn_name, sizeof(request.InterfaceName)); | ||
| memcpy(request.InterfaceName, ifRequest.ifr_ifrn.ifrn_name, sizeof(request.InterfaceName) - 1); | ||
| request.InterfaceName[sizeof(request.InterfaceName) - 1] = '\0'; |
Collaborator
There was a problem hiding this comment.
nit: This can be skipped now that we never write the last byte
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1 of 6 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The half-close behavior needs regression coverage for reverse traffic and final relay termination.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This branch has not been deployed
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.
Summary of the Pull Request
Fix relay loop to track closed states independently for left and right handles. Previously, the loop could exit or misbehave if only one handle was null. This update introduces
leftReadClosedandrightReadClosedboolean flags to properly manage half-closed connections. The relay loop now correctly continues reading from an open handle and only breaks when both the left and right reads are fully closed.PR Checklist
Detailed Description of the Pull Request / Additional comments
Independent close tracking
The previous relay loop did not distinguish between the left and right handles when detecting read-side closures. If one side closed its write direction (half-close) or its read handle became null, the loop would either exit prematurely or behave unpredictably, dropping data that was still buffered on the open side.
This change introduces two explicit boolean flags -
leftReadClosedandrightReadClosed-that are set independently whenPOLLHUP/POLLERRis detected on each respective handle or when the handle becomes null. This allows the loop to continue relaying data in the direction that is still open.Break condition
The exit condition is now gated on both flags being true:
This ensures the relay thread stays alive until both directions have been fully drained, preventing data loss during half-closed or asynchronous teardown scenarios.
Validation Steps Performed