Skip to content

Commit 93bc0af

Browse files
miniksaDHowett
authored andcommitted
Rework locking and eventing during startup and shutdown to alleviate some VT issues (#2525)
Adjusts the startup and shutdown behavior of most threads in the console host to alleviate race conditions that are either exacerbated or introduced by the VT PTY threads. (cherry picked from commit 56c3594)
1 parent a7f03ce commit 93bc0af

32 files changed

+732
-288
lines changed

src/host/PtySignalInputThread.cpp

Lines changed: 22 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,18 @@ using namespace Microsoft::Console::VirtualTerminal;
2626
// - Creates the PTY Signal Input Thread.
2727
// Arguments:
2828
// - hPipe - a handle to the file representing the read end of the VT pipe.
29-
PtySignalInputThread::PtySignalInputThread(_In_ wil::unique_hfile hPipe) :
29+
// - shutdownEvent - An event that will be signaled when the entire console is going down
30+
// We can use this to know when to cancel what we're doing and cleanup.
31+
PtySignalInputThread::PtySignalInputThread(wil::unique_hfile hPipe,
32+
wil::shared_event shutdownEvent) :
3033
_hFile{ std::move(hPipe) },
34+
_shutdownEvent{ shutdownEvent },
3135
_hThread{},
3236
_pConApi{ std::make_unique<ConhostInternalGetSet>(ServiceLocator::LocateGlobals().getConsoleInformation()) },
3337
_dwThreadId{ 0 },
3438
_consoleConnected{ false }
3539
{
40+
THROW_HR_IF(E_HANDLE, !_shutdownEvent);
3641
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
3742
THROW_IF_NULL_ALLOC(_pConApi.get());
3843
}
@@ -79,15 +84,21 @@ void PtySignalInputThread::ConnectConsole() noexcept
7984
// - Otherwise it may cause an application termination another route and never return.
8085
[[nodiscard]] HRESULT PtySignalInputThread::_InputThread()
8186
{
82-
unsigned short signalId;
83-
while (_GetData(&signalId, sizeof(signalId)))
87+
auto onExitTriggerShutdown = wil::scope_exit([&] {
88+
_shutdownEvent.SetEvent();
89+
});
90+
91+
while (true)
8492
{
93+
unsigned short signalId;
94+
RETURN_IF_FAILED(_GetData(&signalId, sizeof(signalId)));
95+
8596
switch (signalId)
8697
{
8798
case PTY_SIGNAL_RESIZE_WINDOW:
8899
{
89100
PTY_SIGNAL_RESIZE resizeMsg = { 0 };
90-
_GetData(&resizeMsg, sizeof(resizeMsg));
101+
RETURN_IF_FAILED(_GetData(&resizeMsg, sizeof(resizeMsg)));
91102

92103
LockConsole();
93104
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
@@ -117,7 +128,7 @@ void PtySignalInputThread::ConnectConsole() noexcept
117128
}
118129
default:
119130
{
120-
THROW_HR(E_UNEXPECTED);
131+
RETURN_HR(E_UNEXPECTED);
121132
}
122133
}
123134
}
@@ -132,34 +143,19 @@ void PtySignalInputThread::ConnectConsole() noexcept
132143
// - cbBuffer - Count of bytes in the given buffer.
133144
// Return Value:
134145
// - True if data was retrieved successfully. False otherwise.
135-
bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
136-
const DWORD cbBuffer)
146+
[[nodiscard]] HRESULT PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
147+
const DWORD cbBuffer)
137148
{
138149
DWORD dwRead = 0;
139150
// If we failed to read because the terminal broke our pipe (usually due
140151
// to dying itself), close gracefully with ERROR_BROKEN_PIPE.
141152
// Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that
142153
// we want to gracefully close in.
143-
if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr))
144-
{
145-
DWORD lastError = GetLastError();
146-
if (lastError == ERROR_BROKEN_PIPE)
147-
{
148-
_Shutdown();
149-
return false;
150-
}
151-
else
152-
{
153-
THROW_WIN32(lastError);
154-
}
155-
}
156-
else if (dwRead != cbBuffer)
157-
{
158-
_Shutdown();
159-
return false;
160-
}
154+
RETURN_IF_WIN32_BOOL_FALSE(ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr));
161155

162-
return true;
156+
RETURN_HR_IF(E_UNEXPECTED, dwRead != cbBuffer);
157+
158+
return S_OK;
163159
}
164160

165161
// Method Description:
@@ -185,31 +181,3 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
185181

186182
return S_OK;
187183
}
188-
189-
// Method Description:
190-
// - Perform a shutdown of the console. This happens when the signal pipe is
191-
// broken, which means either the parent terminal process has died, or they
192-
// called ClosePseudoConsole.
193-
// CloseConsoleProcessState is not enough by itself - it will disconnect
194-
// clients as if the X was pressed, but then we need to actually still die,
195-
// so then call RundownAndExit.
196-
// Arguments:
197-
// - <none>
198-
// Return Value:
199-
// - <none>
200-
void PtySignalInputThread::_Shutdown()
201-
{
202-
// Trigger process shutdown.
203-
CloseConsoleProcessState();
204-
205-
// If we haven't terminated by now, that's because there's a client that's still attached.
206-
// Force the handling of the control events by the attached clients.
207-
// As of MSFT:19419231, CloseConsoleProcessState will make sure this
208-
// happens if this method is called outside of lock, but if we're
209-
// currently locked, we want to make sure ctrl events are handled
210-
// _before_ we RundownAndExit.
211-
ProcessCtrlEvents();
212-
213-
// Make sure we terminate.
214-
ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
215-
}

src/host/PtySignalInputThread.hpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ namespace Microsoft::Console
2020
class PtySignalInputThread final
2121
{
2222
public:
23-
PtySignalInputThread(_In_ wil::unique_hfile hPipe);
23+
PtySignalInputThread(wil::unique_hfile hPipe,
24+
wil::shared_event shutdownEvent);
2425
~PtySignalInputThread();
2526

2627
[[nodiscard]] HRESULT Start() noexcept;
@@ -34,8 +35,9 @@ namespace Microsoft::Console
3435

3536
private:
3637
[[nodiscard]] HRESULT _InputThread();
37-
bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
38-
void _Shutdown();
38+
[[nodiscard]] HRESULT _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
39+
40+
wil::shared_event _shutdownEvent;
3941

4042
wil::unique_hfile _hFile;
4143
wil::unique_handle _hThread;

src/host/VtInputThread.cpp

Lines changed: 82 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,16 @@ using namespace Microsoft::Console::VirtualTerminal;
2424
// - hPipe - a handle to the file representing the read end of the VT pipe.
2525
// - inheritCursor - a bool indicating if the state machine should expect a
2626
// cursor positioning sequence. See MSFT:15681311.
27-
VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
27+
VtInputThread::VtInputThread(wil::unique_hfile hPipe,
28+
wil::shared_event shutdownEvent,
2829
const bool inheritCursor) :
2930
_hFile{ std::move(hPipe) },
31+
_shutdownEvent{ shutdownEvent },
3032
_hThread{},
3133
_utf8Parser{ CP_UTF8 },
32-
_dwThreadId{ 0 },
33-
_exitRequested{ false },
34-
_exitResult{ S_OK }
34+
_dwThreadId{ 0 }
3535
{
36+
THROW_HR_IF(E_HANDLE, !_shutdownEvent);
3637
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
3738

3839
CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
@@ -45,6 +46,30 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
4546

4647
_pInputStateMachine = std::make_unique<StateMachine>(engine.release());
4748
THROW_IF_NULL_ALLOC(_pInputStateMachine.get());
49+
50+
_shutdownWatchdog = std::async(std::launch::async, [&] {
51+
_shutdownEvent.wait();
52+
if (_dwThreadId != 0)
53+
{
54+
wil::unique_handle threadHandle(OpenThread(STANDARD_RIGHTS_ALL | THREAD_TERMINATE, FALSE, _dwThreadId));
55+
LOG_LAST_ERROR_IF_NULL(threadHandle.get());
56+
if (threadHandle)
57+
{
58+
LOG_IF_WIN32_BOOL_FALSE(CancelSynchronousIo(threadHandle.get()));
59+
}
60+
}
61+
});
62+
}
63+
64+
VtInputThread::~VtInputThread()
65+
{
66+
if (_shutdownEvent)
67+
{
68+
_shutdownEvent.SetEvent();
69+
70+
// Wait for watchdog future to get the memo or we might try to destroy it before it gets to work.
71+
_shutdownWatchdog.wait();
72+
}
4873
}
4974

5075
// Method Description:
@@ -96,44 +121,54 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
96121
return pInstance->_InputThread();
97122
}
98123

99-
// Method Description:
100-
// - Do a single ReadFile from our pipe, and try and handle it. If handling
101-
// failed, throw or log, depending on what the caller wants.
124+
// Routine Description:
125+
// - A public way of pumping a single input message through the VT input channel
126+
// - Reading input can be a blocking operation. This function will capture
127+
// the thread ID of whomever calls it so it can be unblocked on shutdown events
128+
// by a watchdog thread.
129+
// - This function cannot be called by two public methods simultaneously.
130+
// If another is already waiting in a blocked read on the VT input thread,
131+
// an invalid state error will be returned.
132+
// - This function is only valid during startup. Once the real VtInputThread starts
133+
// to process the input, it will fill the thread ID field permanently until shutdown.
102134
// Arguments:
103-
// - throwOnFail: If true, throw an exception if there was an error processing
104-
// the input recieved. Otherwise, log the error.
135+
// - <none>
105136
// Return Value:
137+
// - S_OK, a ReadFile error, an error processing input, or an invalid state error if another thread is already waiting.
138+
[[nodiscard]] HRESULT VtInputThread::DoReadInput()
139+
{
140+
// If there's already a thread pumping VT input, it's not valid to read this from the outside.
141+
RETURN_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), _dwThreadId != 0);
142+
143+
// Store which thread is attempting to read VT input. It may get blocked indefinitely and need
144+
// to get unstuck by a shutdown event.
145+
_dwThreadId = GetCurrentThreadId();
146+
147+
// Set it back to 0 on the way out.
148+
auto restoreThreadId = wil::scope_exit([&] {
149+
_dwThreadId = 0;
150+
});
151+
152+
// Perform the blocking read operation.
153+
return _ReadInput();
154+
}
155+
156+
// Method Description:
157+
// - Do a single ReadFile from our pipe, and try and handle it.
158+
// Arguments:
106159
// - <none>
107-
void VtInputThread::DoReadInput(const bool throwOnFail)
160+
// Return Value:
161+
// - S_OK or relevant error
162+
[[nodiscard]] HRESULT VtInputThread::_ReadInput()
108163
{
109164
byte buffer[256];
110165
DWORD dwRead = 0;
111-
bool fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);
112166

113-
// If we failed to read because the terminal broke our pipe (usually due
114-
// to dying itself), close gracefully with ERROR_BROKEN_PIPE.
115-
// Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that
116-
// we want to gracefully close in.
117-
if (!fSuccess)
118-
{
119-
_exitRequested = true;
120-
_exitResult = HRESULT_FROM_WIN32(GetLastError());
121-
return;
122-
}
167+
RETURN_IF_WIN32_BOOL_FALSE(ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr));
123168

124-
HRESULT hr = _HandleRunInput(buffer, dwRead);
125-
if (FAILED(hr))
126-
{
127-
if (throwOnFail)
128-
{
129-
_exitResult = hr;
130-
_exitRequested = true;
131-
}
132-
else
133-
{
134-
LOG_IF_FAILED(hr);
135-
}
136-
}
169+
RETURN_IF_FAILED(_HandleRunInput(buffer, dwRead));
170+
171+
return S_OK;
137172
}
138173

139174
// Method Description:
@@ -145,13 +180,19 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
145180
// have caused us to exit.
146181
DWORD VtInputThread::_InputThread()
147182
{
148-
while (!_exitRequested)
183+
auto onExitTriggerShutdown = wil::scope_exit([&] {
184+
_shutdownEvent.SetEvent();
185+
});
186+
187+
while (true)
149188
{
150-
DoReadInput(true);
189+
// NOTE: From inside the thread itself, we don't need to stash the thread handle each call
190+
// because it was done permanently for us when the thread was created. No one else is allowed
191+
// in through the public method while the actual VtInputThread is running. Only during startup.
192+
RETURN_IF_FAILED(_ReadInput());
151193
}
152-
ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput();
153194

154-
return _exitResult;
195+
return S_OK;
155196
}
156197

157198
// Method Description:
@@ -173,6 +214,9 @@ DWORD VtInputThread::_InputThread()
173214

174215
RETURN_LAST_ERROR_IF_NULL(hThread);
175216
_hThread.reset(hThread);
217+
218+
// This will permanently shut the door on the public read method until shutdown.
219+
// Once the thread is servicing messages, we don't want any other threads getting in here.
176220
_dwThreadId = dwThreadId;
177221

178222
return S_OK;

src/host/VtInputThread.hpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,23 +22,29 @@ namespace Microsoft::Console
2222
class VtInputThread
2323
{
2424
public:
25-
VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor);
25+
VtInputThread(wil::unique_hfile hPipe,
26+
wil::shared_event shutdownEvent,
27+
const bool inheritCursor);
28+
29+
~VtInputThread();
2630

2731
[[nodiscard]] HRESULT Start();
2832
static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter);
29-
void DoReadInput(const bool throwOnFail);
33+
34+
[[nodiscard]] HRESULT DoReadInput();
3035

3136
private:
3237
[[nodiscard]] HRESULT _HandleRunInput(_In_reads_(cch) const byte* const charBuffer, const int cch);
3338
DWORD _InputThread();
39+
[[nodiscard]] HRESULT _ReadInput();
40+
41+
wil::shared_event _shutdownEvent;
42+
std::future<void> _shutdownWatchdog;
3443

3544
wil::unique_hfile _hFile;
3645
wil::unique_handle _hThread;
3746
DWORD _dwThreadId;
3847

39-
bool _exitRequested;
40-
HRESULT _exitResult;
41-
4248
std::unique_ptr<Microsoft::Console::VirtualTerminal::StateMachine> _pInputStateMachine;
4349
Utf8ToWideCharParser _utf8Parser;
4450
};

0 commit comments

Comments
 (0)