diff --git a/src/win/conpty.cc b/src/win/conpty.cc index 931c5362d..748e66c0d 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -47,6 +47,8 @@ struct pty_baton { HPCON hpc; HANDLE hShell = nullptr; + // How to close hpc, for the exit watcher, which has no useConptyDll of its own + PFNCLOSEPSEUDOCONSOLE pfnClosePseudoConsole = nullptr; pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; @@ -69,6 +71,10 @@ static pty_baton* get_pty_baton(const std::lock_guard&, int id) { struct ExitEvent { int exit_code = 0; + // The pseudoconsole a shell that exited on its own leaves open, null if + // kill() closed it first + HPCON hpc = nullptr; + PFNCLOSEPSEUDOCONSOLE pfnClosePseudoConsole = nullptr; }; void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { @@ -87,6 +93,23 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { *th = std::thread([tsfn = std::move(tsfn), baton] { auto callback = [](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) { cb.Call({Napi::Number::New(env, exit_event->exit_code)}); + // Closed only once JS has the exit code, so it has it before the output + // pipe closes, and on a thread of its own: before Windows 11 24H2, + // ClosePseudoConsole waits until the output pipe is drained, and a paused + // pty's output drains only as the main thread reads it. + if (exit_event->hpc != nullptr && exit_event->pfnClosePseudoConsole != nullptr) { + // The thread runs this module's code, so pin the module: a Worker that + // loaded node-pty unloads it when it ends, maybe mid-close. + static std::once_flag pinned; + std::call_once(pinned, [] { + HMODULE module; + GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, + reinterpret_cast(&SetupExitCallback), &module); + }); + std::thread([hpc = exit_event->hpc, pfnClose = exit_event->pfnClosePseudoConsole] { + pfnClose(hpc); + }).detach(); + } delete exit_event; }; @@ -97,6 +120,11 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { std::lock_guard lock(g_ptyHandlesMutex); GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); CloseHandle(baton->hShell); + // kill() can't find the baton once it's erased, so the exit callback + // closes the pseudoconsole unless kill() already has. + exit_event->hpc = baton->hpc; + exit_event->pfnClosePseudoConsole = baton->pfnClosePseudoConsole; + baton->hpc = nullptr; const int id = baton->id; std::erase_if(ptyHandles, [id](const auto& ptyHandle) { return ptyHandle->id == id; @@ -459,6 +487,13 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { } } + if (fLoadedDll) + { + handle->pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( + (HMODULE)hLibrary, + useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); + } + // Update handle handle->hShell = piClient.hProcess; @@ -577,9 +612,12 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( (HMODULE)hLibrary, useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); - if (pfnClosePseudoConsole) + if (pfnClosePseudoConsole && handle->hpc != nullptr) { pfnClosePseudoConsole(handle->hpc); + // The handle stays in ptyHandles until the exit watcher erases it, so + // null hpc to keep a later call from using the closed pseudoconsole. + handle->hpc = nullptr; } } // Defensive: if PtyConnect was never called (or failed before closing the diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index c76196512..85942cc95 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -147,7 +147,8 @@ if (process.platform === 'win32') { describe('pid', () => { it('should be 0 before ready and set after ready_datapipe (issue #763)', function (done) { this.timeout(10000); - const term = new WindowsTerminal('cmd.exe', '/c echo test', { useConptyDll }); + // A shell that waits for kill(), so it's still running when the pid is checked + const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); // pid may be 0 immediately after construction due to deferred connection const initialPid = term.pid; @@ -270,6 +271,48 @@ if (process.platform === 'win32') { done(); }); }); + + it('should close the pseudoconsole when the shell exits on its own (#965)', function (done) { + this.timeout(20000); + const hostName = useConptyDll ? 'OpenConsole.exe' : 'conhost.exe'; + const hosts = (): Promise => psList({ all: true }).then(ps => + ps.filter(p => p.ppid === process.pid && p.name === hostName).map(p => p.pid)); + hosts().then(before => { + const term = new WindowsTerminal('cmd.exe', '', { useConptyDll }); + hosts().then(after => { + const own = after.filter(pid => !before.includes(pid)); + assert.strictEqual(own.length, 1); + term.on('exit', () => { + const gone: IProcessState = {}; + own.forEach(pid => gone[pid] = false); + pollForProcessState(gone, 200, 5000).then(() => done()); + }); + term.write('exit\r'); + }); + }); + }); + + it('should not close the pseudoconsole twice when kill() races the exit', function (done) { + this.timeout(30000); + const N = 10; + let exited = 0; + for (let i = 0; i < N; i++) { + const term = new WindowsTerminal('cmd.exe', '/C exit', { useConptyDll }); + let termExited = false; + term.once('exit', () => { + termExited = true; + if (++exited === N) { + done(); + } + }); + // Only until its exit arrives: a kill() after that isn't safe (#967) + setTimeout(() => { + if (!termExited) { + term.kill(); + } + }, i * 10); + } + }); }); describe('Write', () => {