connection: run all pseudoconsole hosts in jobs (#970)

* Connection: run all pseudoconsole hosts in jobs

This commit also switches the manual resource management in
ConhostConnection to use WIL, and modernizes the constructor to follow
new code style guidelines.

* Terminate conhost before trying to run down the pipes

src/cascadia/TerminalConnection/ConhostConnection.cpp

@@ -31,7 +31,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         _startingDirectory{ startingDirectory },
         _guid{ initialGuid }
     {
-        if (_guid == guid())
+        if (_guid == guid{})
         {
             _guid = Utils::CreateGuid();
         }
@@ -92,31 +92,51 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
                          &_outPipe,
                          &_signalPipe,
                          &_piConhost,
+                         CREATE_SUSPENDED,
                          extraEnvVars));
 
-        _connected = true;
+        _hJob.reset(CreateJobObjectW(nullptr, nullptr));
+        THROW_LAST_ERROR_IF_NULL(_hJob);
+
+        // We want the conhost and all associated descendant processes
+        // to be terminated when the tab is closed. GUI applications
+        // spawned from the shell tend to end up in their own jobs.
+        JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobExtendedInformation{};
+        jobExtendedInformation.BasicLimitInformation.LimitFlags =
+            JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+
+        THROW_IF_WIN32_BOOL_FALSE(SetInformationJobObject(_hJob.get(),
+                                                          JobObjectExtendedLimitInformation,
+                                                          &jobExtendedInformation,
+                                                          sizeof(jobExtendedInformation)));
+
+        THROW_IF_WIN32_BOOL_FALSE(AssignProcessToJobObject(_hJob.get(), _piConhost.hProcess));
 
         // Create our own output handling thread
-        // Each console needs to make sure to drain the output from its backing host.
-        _outputThreadId = static_cast<DWORD>(-1);
-        _hOutputThread = CreateThread(nullptr,
-                                      0,
-                                      StaticOutputThreadProc,
-                                      this,
-                                      0,
-                                      &_outputThreadId);
+        // Each connection needs to make sure to drain the output from its backing host.
+        _hOutputThread.reset(CreateThread(nullptr,
+                                          0,
+                                          StaticOutputThreadProc,
+                                          this,
+                                          0,
+                                          nullptr));
+
+        // Wind up the conhost! We only do this after we've got everything in place.
+        THROW_LAST_ERROR_IF(-1 == ResumeThread(_piConhost.hThread));
+
+        _connected = true;
     }
 
     void ConhostConnection::WriteInput(hstring const& data)
     {
-        if (!_connected || _closing)
+        if (!_connected || _closing.load())
         {
             return;
         }
 
         // convert from UTF-16LE to UTF-8 as ConPty expects UTF-8
         std::string str = winrt::to_string(data);
-        bool fSuccess = !!WriteFile(_inPipe, str.c_str(), (DWORD)str.length(), nullptr, nullptr);
+        bool fSuccess = !!WriteFile(_inPipe.get(), str.c_str(), (DWORD)str.length(), nullptr, nullptr);
         fSuccess;
     }
 
@@ -127,9 +147,9 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             _initialRows = rows;
             _initialCols = columns;
         }
-        else if (!_closing)
+        else if (!_closing.load())
         {
-            SignalResizeWindow(_signalPipe, static_cast<unsigned short>(columns), static_cast<unsigned short>(rows));
+            SignalResizeWindow(_signalPipe.get(), Utils::ClampToShortMax(columns, 1), Utils::ClampToShortMax(rows, 1));
         }
     }
 
@@ -139,24 +159,19 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         {
             return;
         }
-        if (_closing)
+
+        if (!_closing.exchange(true))
         {
-            return;
+            _hJob.reset(); // This will terminate the process _piConhost is holding.
+            _piConhost.reset();
+
+            _inPipe.reset();
+            _outPipe.reset();
+            _signalPipe.reset();
+
+            WaitForSingleObject(_hOutputThread.get(), INFINITE);
+            _hOutputThread.reset();
         }
-        _closing = true;
-        _connected = false;
-
-        // Terminate the output thread
-        _outputThreadId = 0;
-        TerminateThread(_hOutputThread, 0);
-
-        // Close our pipes and the pseudoconsole
-        CloseHandle(_signalPipe);
-        CloseHandle(_inPipe);
-        CloseHandle(_outPipe);
-
-        TerminateProcess(_piConhost.hProcess, 0);
-        CloseHandle(_piConhost.hProcess);
     }
 
     DWORD WINAPI ConhostConnection::StaticOutputThreadProc(LPVOID lpParameter)
@@ -175,10 +190,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             dwRead = 0;
             bool fSuccess = false;
 
-            fSuccess = !!ReadFile(_outPipe, buffer, bufferSize, &dwRead, nullptr);
+            fSuccess = !!ReadFile(_outPipe.get(), buffer, bufferSize, &dwRead, nullptr);
             if (!fSuccess)
             {
-                if (_closing)
+                if (_closing.load())
                 {
                     // This is okay, break out to kill the thread
                     return 0;

src/cascadia/TerminalConnection/ConhostConnection.h

@@ -28,18 +28,19 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
         uint32_t _initialRows{};
         uint32_t _initialCols{};
-        hstring _commandline{};
-        hstring _startingDirectory{};
+        hstring _commandline;
+        hstring _startingDirectory;
+        guid _guid{}; // A unique session identifier for connected client
 
         bool _connected{};
-        HANDLE _inPipe{ INVALID_HANDLE_VALUE }; // The pipe for writing input to
-        HANDLE _outPipe{ INVALID_HANDLE_VALUE }; // The pipe for reading output from
-        HANDLE _signalPipe{ INVALID_HANDLE_VALUE };
-        DWORD _outputThreadId{};
-        HANDLE _hOutputThread{ nullptr };
-        PROCESS_INFORMATION _piConhost{};
-        guid _guid{}; // A "unique" session identifier for connected client
-        bool _closing{};
+        std::atomic<bool> _closing{ false };
+
+        wil::unique_hfile _inPipe; // The pipe for writing input to
+        wil::unique_hfile _outPipe; // The pipe for reading output from
+        wil::unique_hfile _signalPipe;
+        wil::unique_handle _hOutputThread;
+        wil::unique_process_information _piConhost;
+        wil::unique_handle _hJob;
 
         static DWORD WINAPI StaticOutputThreadProc(LPVOID lpParameter);
         DWORD _OutputThread();

src/inc/conpty-universal.h

@@ -56,6 +56,7 @@ using EnvironmentVariableMapW = std::map<std::wstring, std::wstring, WStringCase
                                    HANDLE* const hOutput,
                                    HANDLE* const hSignal,
                                    PROCESS_INFORMATION* const piPty,
+                                   DWORD dwCreationFlags = 0,
                                    const EnvironmentVariableMapW& extraEnvVars = {}) noexcept;
 
 bool SignalResizeWindow(const HANDLE hSignal,
@@ -219,6 +220,7 @@ bool SignalResizeWindow(const HANDLE hSignal,
                                                                HANDLE* const hOutput,
                                                                HANDLE* const hSignal,
                                                                PROCESS_INFORMATION* const piPty,
+                                                               DWORD dwCreationFlags = 0,
                                                                const EnvironmentVariableMapW& extraEnvVars = {}) noexcept
 {
     // Create some anon pipes so we can pass handles down and into the console.
@@ -292,7 +294,6 @@ bool SignalResizeWindow(const HANDLE hSignal,
                            newEnvVars.size() * sizeof(decltype(newEnvVars.begin())::value_type));
     });
 
-    DWORD dwCreationFlags = 0;
     if (!extraEnvVars.empty())
     {
         EnvironmentVariableMapW tempEnvMap{ extraEnvVars };