Skip to content

Commit

Permalink
Rework locking and eventing during startup and shutdown to alleviate …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@miniksa
miniksa committed Sep 20, 2019
1 parent 9102c5d commit 56c3594
Show file tree
Hide file tree
Showing 32 changed files with 732 additions and 288 deletions.
76 changes: 22 additions & 54 deletions src/host/PtySignalInputThread.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,18 @@ using namespace Microsoft::Console::VirtualTerminal;
// - Creates the PTY Signal Input Thread.
// Arguments:
// - hPipe - a handle to the file representing the read end of the VT pipe.
PtySignalInputThread::PtySignalInputThread(_In_ wil::unique_hfile hPipe) :
// - shutdownEvent - An event that will be signaled when the entire console is going down
// We can use this to know when to cancel what we're doing and cleanup.
PtySignalInputThread::PtySignalInputThread(wil::unique_hfile hPipe,
wil::shared_event shutdownEvent) :
_hFile{ std::move(hPipe) },
_shutdownEvent{ shutdownEvent },
_hThread{},
_pConApi{ std::make_unique<ConhostInternalGetSet>(ServiceLocator::LocateGlobals().getConsoleInformation()) },
_dwThreadId{ 0 },
_consoleConnected{ false }
{
THROW_HR_IF(E_HANDLE, !_shutdownEvent);
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
THROW_IF_NULL_ALLOC(_pConApi.get());
}
Expand Down Expand Up @@ -79,15 +84,21 @@ void PtySignalInputThread::ConnectConsole() noexcept
// - Otherwise it may cause an application termination another route and never return.
[[nodiscard]] HRESULT PtySignalInputThread::_InputThread()
{
unsigned short signalId;
while (_GetData(&signalId, sizeof(signalId)))
auto onExitTriggerShutdown = wil::scope_exit([&] {
_shutdownEvent.SetEvent();
});

while (true)
{
unsigned short signalId;
RETURN_IF_FAILED(_GetData(&signalId, sizeof(signalId)));

switch (signalId)
{
case PTY_SIGNAL_RESIZE_WINDOW:
{
PTY_SIGNAL_RESIZE resizeMsg = { 0 };
_GetData(&resizeMsg, sizeof(resizeMsg));
RETURN_IF_FAILED(_GetData(&resizeMsg, sizeof(resizeMsg)));

LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
Expand Down Expand Up @@ -117,7 +128,7 @@ void PtySignalInputThread::ConnectConsole() noexcept
}
default:
{
THROW_HR(E_UNEXPECTED);
RETURN_HR(E_UNEXPECTED);
}
}
}
Expand All @@ -132,34 +143,19 @@ void PtySignalInputThread::ConnectConsole() noexcept
// - cbBuffer - Count of bytes in the given buffer.
// Return Value:
// - True if data was retrieved successfully. False otherwise.
bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
const DWORD cbBuffer)
[[nodiscard]] HRESULT PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
const DWORD cbBuffer)
{
DWORD dwRead = 0;
// If we failed to read because the terminal broke our pipe (usually due
// to dying itself), close gracefully with ERROR_BROKEN_PIPE.
// Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that
// we want to gracefully close in.
if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr))
{
DWORD lastError = GetLastError();
if (lastError == ERROR_BROKEN_PIPE)
{
_Shutdown();
return false;
}
else
{
THROW_WIN32(lastError);
}
}
else if (dwRead != cbBuffer)
{
_Shutdown();
return false;
}
RETURN_IF_WIN32_BOOL_FALSE(ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr));

return true;
RETURN_HR_IF(E_UNEXPECTED, dwRead != cbBuffer);

return S_OK;
}

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

return S_OK;
}

// Method Description:
// - Perform a shutdown of the console. This happens when the signal pipe is
// broken, which means either the parent terminal process has died, or they
// called ClosePseudoConsole.
// CloseConsoleProcessState is not enough by itself - it will disconnect
// clients as if the X was pressed, but then we need to actually still die,
// so then call RundownAndExit.
// Arguments:
// - <none>
// Return Value:
// - <none>
void PtySignalInputThread::_Shutdown()
{
// Trigger process shutdown.
CloseConsoleProcessState();

// If we haven't terminated by now, that's because there's a client that's still attached.
// Force the handling of the control events by the attached clients.
// As of MSFT:19419231, CloseConsoleProcessState will make sure this
// happens if this method is called outside of lock, but if we're
// currently locked, we want to make sure ctrl events are handled
// _before_ we RundownAndExit.
ProcessCtrlEvents();

// Make sure we terminate.
ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
}
8 changes: 5 additions & 3 deletions src/host/PtySignalInputThread.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ namespace Microsoft::Console
class PtySignalInputThread final
{
public:
PtySignalInputThread(_In_ wil::unique_hfile hPipe);
PtySignalInputThread(wil::unique_hfile hPipe,
wil::shared_event shutdownEvent);
~PtySignalInputThread();

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

private:
[[nodiscard]] HRESULT _InputThread();
bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
void _Shutdown();
[[nodiscard]] HRESULT _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);

wil::shared_event _shutdownEvent;

wil::unique_hfile _hFile;
wil::unique_handle _hThread;
Expand Down
120 changes: 82 additions & 38 deletions src/host/VtInputThread.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ using namespace Microsoft::Console::VirtualTerminal;
// - hPipe - a handle to the file representing the read end of the VT pipe.
// - inheritCursor - a bool indicating if the state machine should expect a
// cursor positioning sequence. See MSFT:15681311.
VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
VtInputThread::VtInputThread(wil::unique_hfile hPipe,
wil::shared_event shutdownEvent,
const bool inheritCursor) :
_hFile{ std::move(hPipe) },
_shutdownEvent{ shutdownEvent },
_hThread{},
_utf8Parser{ CP_UTF8 },
_dwThreadId{ 0 },
_exitRequested{ false },
_exitResult{ S_OK }
_dwThreadId{ 0 }
{
THROW_HR_IF(E_HANDLE, !_shutdownEvent);
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);

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

_pInputStateMachine = std::make_unique<StateMachine>(engine.release());
THROW_IF_NULL_ALLOC(_pInputStateMachine.get());

_shutdownWatchdog = std::async(std::launch::async, [&] {
_shutdownEvent.wait();
if (_dwThreadId != 0)
{
wil::unique_handle threadHandle(OpenThread(STANDARD_RIGHTS_ALL | THREAD_TERMINATE, FALSE, _dwThreadId));
LOG_LAST_ERROR_IF_NULL(threadHandle.get());
if (threadHandle)
{
LOG_IF_WIN32_BOOL_FALSE(CancelSynchronousIo(threadHandle.get()));
}
}
});
}

VtInputThread::~VtInputThread()
{
if (_shutdownEvent)
{
_shutdownEvent.SetEvent();

// Wait for watchdog future to get the memo or we might try to destroy it before it gets to work.
_shutdownWatchdog.wait();
}
}

// Method Description:
Expand Down Expand Up @@ -96,44 +121,54 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
return pInstance->_InputThread();
}

// Method Description:
// - Do a single ReadFile from our pipe, and try and handle it. If handling
// failed, throw or log, depending on what the caller wants.
// Routine Description:
// - A public way of pumping a single input message through the VT input channel
// - Reading input can be a blocking operation. This function will capture
// the thread ID of whomever calls it so it can be unblocked on shutdown events
// by a watchdog thread.
// - This function cannot be called by two public methods simultaneously.
// If another is already waiting in a blocked read on the VT input thread,
// an invalid state error will be returned.
// - This function is only valid during startup. Once the real VtInputThread starts
// to process the input, it will fill the thread ID field permanently until shutdown.
// Arguments:
// - throwOnFail: If true, throw an exception if there was an error processing
// the input recieved. Otherwise, log the error.
// - <none>
// Return Value:
// - S_OK, a ReadFile error, an error processing input, or an invalid state error if another thread is already waiting.
[[nodiscard]] HRESULT VtInputThread::DoReadInput()
{
// If there's already a thread pumping VT input, it's not valid to read this from the outside.
RETURN_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), _dwThreadId != 0);

// Store which thread is attempting to read VT input. It may get blocked indefinitely and need
// to get unstuck by a shutdown event.
_dwThreadId = GetCurrentThreadId();

// Set it back to 0 on the way out.
auto restoreThreadId = wil::scope_exit([&] {
_dwThreadId = 0;
});

// Perform the blocking read operation.
return _ReadInput();
}

// Method Description:
// - Do a single ReadFile from our pipe, and try and handle it.
// Arguments:
// - <none>
void VtInputThread::DoReadInput(const bool throwOnFail)
// Return Value:
// - S_OK or relevant error
[[nodiscard]] HRESULT VtInputThread::_ReadInput()
{
byte buffer[256];
DWORD dwRead = 0;
bool fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);

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

HRESULT hr = _HandleRunInput(buffer, dwRead);
if (FAILED(hr))
{
if (throwOnFail)
{
_exitResult = hr;
_exitRequested = true;
}
else
{
LOG_IF_FAILED(hr);
}
}
RETURN_IF_FAILED(_HandleRunInput(buffer, dwRead));

return S_OK;
}

// Method Description:
Expand All @@ -145,13 +180,19 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
// have caused us to exit.
DWORD VtInputThread::_InputThread()
{
while (!_exitRequested)
auto onExitTriggerShutdown = wil::scope_exit([&] {
_shutdownEvent.SetEvent();
});

while (true)
{
DoReadInput(true);
// NOTE: From inside the thread itself, we don't need to stash the thread handle each call
// because it was done permanently for us when the thread was created. No one else is allowed
// in through the public method while the actual VtInputThread is running. Only during startup.
RETURN_IF_FAILED(_ReadInput());
}
ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput();

return _exitResult;
return S_OK;
}

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

RETURN_LAST_ERROR_IF_NULL(hThread);
_hThread.reset(hThread);

// This will permanently shut the door on the public read method until shutdown.
// Once the thread is servicing messages, we don't want any other threads getting in here.
_dwThreadId = dwThreadId;

return S_OK;
Expand Down
16 changes: 11 additions & 5 deletions src/host/VtInputThread.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,29 @@ namespace Microsoft::Console
class VtInputThread
{
public:
VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor);
VtInputThread(wil::unique_hfile hPipe,
wil::shared_event shutdownEvent,
const bool inheritCursor);

~VtInputThread();

[[nodiscard]] HRESULT Start();
static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter);
void DoReadInput(const bool throwOnFail);

[[nodiscard]] HRESULT DoReadInput();

private:
[[nodiscard]] HRESULT _HandleRunInput(_In_reads_(cch) const byte* const charBuffer, const int cch);
DWORD _InputThread();
[[nodiscard]] HRESULT _ReadInput();

wil::shared_event _shutdownEvent;
std::future<void> _shutdownWatchdog;

wil::unique_hfile _hFile;
wil::unique_handle _hThread;
DWORD _dwThreadId;

bool _exitRequested;
HRESULT _exitResult;

std::unique_ptr<Microsoft::Console::VirtualTerminal::StateMachine> _pInputStateMachine;
Utf8ToWideCharParser _utf8Parser;
};
Expand Down
Loading

0 comments on commit 56c3594

Please sign in to comment.