Repository navigation
Reject Session.Close from Terminated handlers - #42144
Kevin Vega (kvega005) merged 1 commit into
Conversation
Co-authored-by: Copilot <[email protected]> Copilot-Session: 99ebbb28-3fda-402d-8b3e-85813bfa8dbc
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the termination-handler deadlock fix.
No open issues 🎉
What changed in this PR
Prevents Session.Close() from deadlocking when called from a Terminated handler or its synchronous COM call chain, while preserving the wait for active handlers when called externally.
Changes:
- Tracks physical and COM logical thread IDs during termination-event dispatch.
- Rejects handler-originated
Close()calls withE_ILLEGAL_METHOD_CALLbefore releasing resources. - Adds regression tests for direct calls, cross-apartment calls, and external close waiting.
| File | Description |
|---|---|
test/windows/wslc/WslcSdkWinRTTests.cpp |
Adds three tests covering close rejection and handler-draining behavior. |
src/windows/WslcSDK/winrt/Session.h |
Declares the handler-context check and lock-protected thread IDs. |
src/windows/WslcSDK/winrt/Session.cpp |
Tracks handler execution context and rejects close calls that would wait on themselves. |
Review effort: Balanced
| // Close waits for in-flight Terminated handlers, so calling it from one would wait on itself. | ||
| if (IsInTerminatedHandler()) | ||
| { | ||
| throw winrt::hresult_illegal_method_call(L"Session cannot be closed from its Terminated handler"); |
There was a problem hiding this comment.
Should localize this message
There was a problem hiding this comment.
We typically don't localize winrt exceptions so I think english here is OK since this isn't designed to be shown to the end user
David Bennett (dkbennett)
left a comment
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
Looks good overall, just a minor localization comment.
ranm-msft
left a comment
ranm-msft
left a comment
There was a problem hiding this comment.
Please cover final-reference release in the termination callback before approval.
| void Session::Close() | ||
| { | ||
| // Close waits for in-flight Terminated handlers, so calling it from one would wait on itself. | ||
| if (IsInTerminatedHandler()) |
There was a problem hiding this comment.
Releasing the last external Session wrapper from a Terminated handler also reaches this guard through final_release(), which calls Close(). That throws through C++/WinRT's noexcept Release path and terminates the process; please make callback/final-release lifetime safe and add a test that clears the last wrapper reference inside the handler.
There was a problem hiding this comment.
I actually discussed this with Blue (@OneBlue) before I sent out the PR. The consensus was "this might be the kind of thing that we document as not supported, and an app crashing because of this would be a user error."
Summary of the Pull Request
Session.Close()now rejects calls from aTerminatedhandler withE_ILLEGAL_METHOD_CALLinstead of deadlocking. Calls from outside the handler still wait for active termination handlers to finish.PR Checklist
Detailed Description of the Pull Request / Additional comments
Problem.
Close()drains the threadpool wait that invokesTerminated. Calling it synchronously from a handler therefore waits for that same handler to return. A handler can also call through COM into another apartment, where checking only the physical thread ID would miss the same dependency.Change.
OnTerminated()records its physical and COM logical thread IDs under a short-heldwil::srwlock.Close()checks those IDs and throwswinrt::hresult_illegal_method_callbefore releasing any resources when called from the handler or its synchronous COM call chain. Logical-ID lookup errors propagate throughwinrt::check_hresult.The lock is not held while invoking application handlers. A
wil::scope_exitclears the IDs after event dispatch finishes, including during exception unwinding. The existing wait and resource-release order inClose()are unchanged.Validation Steps Performed
WslcSdkWinRtTests::TerminationHandlerCloseIsRejectedchecks rejection from two handlers, verifies the session remains open after rejection, and closes it from outside the handler.WslcSdkWinRtTests::TerminationHandlerCloseFromStaCallIsRejectedchecks rejection through a synchronous STA context call with different physical thread IDs and matching non-null logical IDs. A direct lambda invocation alone would not exercise logical-thread matching.WslcSdkWinRtTests::CloseWaitsForTerminationHandlerchecks that an outsideClose()waits for an active handler to finish.WSLC-RunTests.ps1 -RepoRoot 'Q:\WSL2' -TestName '*WslcSdkWinRtTests::*Termination*'— passed all four selected tests, including the existing termination-notification test.