Skip to content

PTY Exited event never fires when DuplicateHandle fails, costing a 10s stall per session at shutdown #91

Description

@AThraen

Found during the v0.6.0 pre-release review. Low severity on its own, but it likely compounds #82.

The bug

PseudoTerminal.MonitorExitAsync (added in #63 to fix the handle race in #39) returns early on a failed DuplicateHandlebefore Exited?.Invoke():

if (!DuplicateHandle(GetCurrentProcess(), _hProcess, GetCurrentProcess(),
        out IntPtr waitHandle, 0, false, DUPLICATE_SAME_ACCESS))
{
    Log($"DuplicateHandle failed: {Marshal.GetLastWin32Error()}");
    return;                     // <-- Exited never fires
}
try { ... }
finally { CloseHandle(waitHandle); }
Exited?.Invoke();

Every other path fires it exactly once. This one silently doesn't.

Why it matters

MainWindow.DisposeAndWaitForExitAsync subscribes to Exited and waits on a TaskCompletionSource:

await Task.WhenAny(tcs.Task, Task.Delay(timeoutMs));   // timeoutMs: 10000

If Exited never fires, that session waits out its full 10-second timeout. The disposal loop is sequential, so with N affected sessions it's N × 10s added to shutdown.

Bounded rather than hanging, and DuplicateHandle on a process handle you already own effectively never fails — which is why this is low severity rather than urgent.

Fix

Move the invoke into a finally so every path fires it exactly once:

try
{
    if (!DuplicateHandle(...)) { Log(...); return; }
    try
    {
        await Task.Run(() => WaitForSingleObject(waitHandle, 0xFFFFFFFF));
        if (GetExitCodeProcess(waitHandle, out uint code)) ExitCode = unchecked((int)code);
    }
    finally { CloseHandle(waitHandle); }
}
finally { Exited?.Invoke(); }

Worth a unit test via the IPseudoTerminal seam asserting Exited fires on the failure path too.

Relationship to #82

#82 is about startup/shutdown scaling badly with live session count, attributed mostly to fixed sleeps. This is a second, independent contributor to the same symptom — a conditional 10s per session rather than a fixed delay. Worth ruling in or out while working #82, since with 40 sessions the two are hard to tell apart from the outside.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions