Skip to content

fix: close kqueue fd in SetupExitCallback on macOS#931

Open
codebytere-ant wants to merge 1 commit into
microsoft:mainfrom
codebytere-ant:fix/macos-kqueue-fd-leak
Open

fix: close kqueue fd in SetupExitCallback on macOS#931
codebytere-ant wants to merge 1 commit into
microsoft:mainfrom
codebytere-ant:fix/macos-kqueue-fd-leak

Conversation

@codebytere-ant

Copy link
Copy Markdown

On macOS, SetupExitCallback opens a kqueue() per spawned pty to wait on NOTE_EXIT, but never closes it before the watcher thread returns. This leaks one kqueue file descriptor per pty.spawn() for the lifetime of the host process.

Repro:

const pty = require('node-pty');
const { execSync } = require('child_process');
const kq = () => execSync(`lsof -p ${process.pid} | grep -c KQUEUE`, {encoding:'utf8'}).trim();
(async () => {
  console.log('baseline KQUEUE=' + kq());
  for (let i = 0; i < 20; i++) {
    const p = pty.spawn('/bin/sh', [], { name: 'xterm', cols: 80, rows: 24 });
    await new Promise(r => { p.onExit(r); setTimeout(() => p.kill(), 50); });
  }
  await new Promise(r => setTimeout(r, 500));
  console.log('after 20 cycles KQUEUE=' + kq());  // baseline + 20
})();

The Chromium reference this code is based on (kill_mac.cc) closes the kqueue via ScopedFD; the equivalent here is an explicit close(kq) once the kevent wait completes. close(-1) is a harmless EBADF if kqueue() itself failed, so no guard is needed.

Adds a regression test alongside the ptmx leak test from #882.

cc @deepak1556

SetupExitCallback opens a kqueue() per spawned pty to wait on
NOTE_EXIT, but never closes it before the watcher thread returns,
leaking one kqueue fd per pty.spawn() for the host process lifetime.

The Chromium kill_mac.cc this is based on closes the kqueue via
ScopedFD; the equivalent here is an explicit close(kq) once the
kevent wait completes.
@codebytere-ant

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Anthropic"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant