Skip to content

Fix potential spin in interop message relay - #41319

Open
Feng Wang (chemwolf6922) wants to merge 5 commits into
masterfrom
user/chemwolf6922/fix-potential-spin-in-interop-messages-relay
Open

Fix potential spin in interop message relay#41319
Feng Wang (chemwolf6922) wants to merge 5 commits into
masterfrom
user/chemwolf6922/fix-potential-spin-in-interop-messages-relay

Conversation

@chemwolf6922

@chemwolf6922 Feng Wang (chemwolf6922) commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

The original logic ignores the success result of the initial ReadFile call. If the connection is a socket and its close is not captured by GetOverlappedResult. It could cause this ReadFile call to return success with bytes read == 0. This will put this loop into a spin.

This PR refactors the loop with io::MultiHandleWait.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

This issue is triggered by a race condition. I'm not able to repro the original issue on my machine.
The normal function was tested locally.

@chemwolf6922
Feng Wang (chemwolf6922) requested a review from a team as a code owner August 12, 2026 05:32
Copilot AI lite review requested due to automatic review settings August 12, 2026 05:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes an infinite-spin condition in WSL interop’s control-message relay loop (ProcessInteropMessages) by handling the synchronous ReadFile success case where BytesRead == 0 (EOF). This aligns the synchronous path with the existing overlapped-completion path and prevents runaway CPU usage when the interop control socket reaches EOF.

Changes:

  • Initialize BytesRead and add a unified EOF/failed-read exit check ((!Success) || (BytesRead == 0)) that applies to both synchronous and asynchronous completion paths.
  • Refactor the ERROR_IO_PENDING handling block so overlapped completion updates Success/BytesRead, then uses the shared exit/termination logic.
  • Preserve existing behavior for process-exit handling (retrieving ExitCode, resetting pseudoconsole, and breaking).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings August 12, 2026 05:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 1 out of 1 changed files in this pull request and generated no new comments.

OneBlue
Blue (OneBlue) previously approved these changes Aug 12, 2026

@OneBlue Blue (OneBlue) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Change looks good, minor comments. Feel free to address in a followup

Comment thread src/windows/common/interop.cpp Outdated
Comment thread src/windows/common/interop.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/windows/common/interop.cpp:418

  • MultiHandleWait can deliver the process handle completion and the message-handle read completion in either order. Since processExit() resets Result->PseudoConsole, a subsequent read callback in the same Run() iteration can still attempt ResizePseudoConsole(Result->PseudoConsole.get(), ...) and throw due to a null/closed HPCON. Consider guarding the read callback after process exit so late read completions don’t touch the pseudoconsole.
    DWORD exitCode = 1;
    size_t pendingSize = 0;
    LX_INIT_WINDOW_SIZE_CHANGED pendingMessage{};

    auto processExit = [&] {

src/windows/common/interop.cpp:440

  • On the EOF path (input.empty()), WaitForSingleObject failure is currently surfaced as E_UNEXPECTED via THROW_HR_IF, which drops GetLastError() context. It’s better to explicitly handle WAIT_FAILED with THROW_LAST_ERROR() so failures are diagnosable.
                    const DWORD waitStatus = WaitForSingleObject(Result->Process.get(), 0);
                    if (waitStatus == WAIT_OBJECT_0)
                    {
                        processExit();
                    }
                    else
                    {
                        THROW_HR_IF(E_UNEXPECTED, waitStatus != WAIT_TIMEOUT);
                        if (WI_IsFlagClear(Result->Flags, LX_INIT_CREATE_PROCESS_RESULT_FLAG_GUI_APPLICATION))

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