Fix a ControlCore race condition on connection close #13882
Merged
Conversation
This file contains 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
ghost
added
Area-TerminalConnection
lhecker
force-pushed
the
dev/lhecker/13880-connection-close-race
branch
from
ce0aa9c
to
9cd5ea6
Compare
|
I've asked Leonard for more tests:
|
zadjii-msft
reviewed
Aug 31, 2022
|
But also maybe test blatting |
|
Alright, I finished testing everything and updated the PR message. |
|
ONE FINAL TEST |
|
Nevermind I couldn't even get it to work normally. |
3 tasks
|
Could we get this into 1.16? I think this issue might be the cause for a crash on tab close I'm seeing quite often in debug builds... |
DHowett
reviewed
Sep 6, 2022
Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
DHowett
approved these changes
Sep 6, 2022
ghost
merged commit
|
🎉 Handy links: |
DHowett
pushed a commit
that referenced
this pull request
Oct 12, 2022
As noted by the `winrt::event` documentation: > [...] But for asynchronous events, even after revoking [...], an in-flight > event might reach your object after it has started destructing. This is because while adding/removing/calling event handlers might be thread-safe, there's no guarantee that they run mutually exclusive. This commit fixes the issue by reverting 6f0f245. Since we never checked the result of closing a terminal connection anyways, this commit simply drops the wait on the connection being teared down to ensure #1996 doesn't regress. Closes #13880 ## Validation Steps Performed * Open tab, close tab, open tab, close tab, open tab, close tab * ConPTY ✅ * Azure ✅ * Closing a tab with a huge amount of panes ✅ * Opening a bunch of tabs and then closing the window ✅ * Closing a tab while it's busy with VT ✅ * `wtd -w 0 nt cmd /c exit` ✅ * `wtd -w -1 cmd /c exit` * No WerFault spawns ✅ (cherry picked from commit 666c446) Service-Card-Id: 86178273 Service-Version: 1.15
|
🎉 Handy links: |
ghost
pushed a commit
that referenced
this pull request
Dec 16, 2022
2 new ConPTY APIs were added as part of this commit: * `ClosePseudoConsoleTimeout` Complements `ClosePseudoConsole`, allowing users to override the `INFINITE` wait for OpenConsole to exit with any arbitrary timeout, including 0. * `ConptyReleasePseudoConsole` This releases the `\Reference` handle held by the `HPCON`. While it makes launching further PTY clients via `PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE` impossible, it does allow conhost/OpenConsole to exit naturally once all console clients have disconnected. This makes it unnecessary to having to monitor the exit of the spawned shell/application, just to then call `ClosePseudoConsole`, while carefully continuing to read from the output pipe. Instead users can now just read from the output pipe until it is closed, knowing for sure that no more data will come or clients connect. This is especially useful in combination with `ClosePseudoConsoleTimeout` and a timeout of 0, to allow conhost/OpenConsole to exit asynchronously. These new APIs are used to fix an array of bugs around Windows Terminal exiting either too early or too late. They also make usage of the ConPTY API simpler in most situations (when spawning a single application and waiting for it to exit). Depends on #13882, #14041, #14160, #14282 Closes #4564 Closes #14416 Closes MSFT-42244182 ## Validation Steps Performed * Calling `FreeConsole` in a handoff'd application closes the tab ✅ * Create a .bat file containing only `start /B cmd.exe`. If WT stable is the default terminal the tab closes instantly ✅ With these changes included the tab stays open with a cmd.exe prompt ✅ * New ConPTY tests ✅
This pull request was closed.
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.
As noted by the
winrt::eventdocumentation:This is because while adding/removing/calling event handlers might be
thread-safe, there's no guarantee that they run mutually exclusive.
This commit fixes the issue by reverting 6f0f245. Since we never checked
the result of closing a terminal connection anyways, this commit simply drops
the wait on the connection being teared down to ensure #1996 doesn't regress.
Closes #13880
Validation Steps Performed
wtd -w 0 nt cmd /c exit✅wtd -w -1 cmd /c exit