fix(sigint): make sure we do not add handler twice (#24413)

In the following scenario, we were adding SIGINT handler twice, but
removing it just once:
- Task runner starts testing, creates SigIntWatcher, installs SIGINT
handler.
- Press Ctrl+C, task runner interrupts, disarms SigIntWatcher, SIGINT
handler is not removed due to 1000ms cooldown.
- Task runner starts cleanup, creates SigIntWatcher, installs another
SIGINT handler.
- Cleanup finishes, SigIntWatcher disarms, could remove or not remove
SIGINT handler based on timing (same 1000ms cooldown). In any case, we
have one or two SIGINT handlers still on.
- HTML reporter hangs in onExit, while we still have SIGINT handler up,
so Ctrl+C does not exit.

Regressed in #24265.

packages/playwright-test/src/runner/sigIntWatcher.ts

@@ -46,6 +46,7 @@ export class SigIntWatcher {
 class FixedNodeSIGINTHandler {
   private static _handlers: (() => void)[] = [];
   private static _ignoreNextSIGINTs = false;
+  private static _handlerInstalled = false;
 
   static _dispatch = () => {
     if (this._ignoreNextSIGINTs)
@@ -70,21 +71,35 @@ class FixedNodeSIGINTHandler {
       // The side effect is that slow shutdown or bug in our process will force
       // the user to hit Ctrl+C again after at least a second.
       if (!this._handlers.length)
-        process.off('SIGINT', this._dispatch);
+        this._uninstall();
     }, 1000);
     for (const handler of this._handlers)
       handler();
   };
 
+  static _install() {
+    if (!this._handlerInstalled) {
+      this._handlerInstalled = true;
+      process.on('SIGINT', this._dispatch);
+    }
+  }
+
+  static _uninstall() {
+    if (this._handlerInstalled) {
+      this._handlerInstalled = false;
+      process.off('SIGINT', this._dispatch);
+    }
+  }
+
   static on(handler: () => void) {
     this._handlers.push(handler);
     if (this._handlers.length === 1)
-      process.on('SIGINT', this._dispatch);
+      this._install();
   }
 
   static off(handler: () => void) {
     this._handlers = this._handlers.filter(h => h !== handler);
     if (!this._ignoreNextSIGINTs && !this._handlers.length)
-      process.off('SIGINT', this._dispatch);
+      this._uninstall();
   }
 }

tests/playwright-test/runner.spec.ts

@@ -581,3 +581,114 @@ test('should not hang on worker error in test file', async ({ runInlineTest }) =
   expect(result.results[0].error.message).toContain('Internal error: worker process exited unexpectedly');
   expect(result.results[1].status).toBe('skipped');
 });
+
+test('fast double SIGINT should be ignored', async ({ interactWithTestRunner }) => {
+  test.skip(process.platform === 'win32', 'No sending SIGINT on Windows');
+
+  const testProcess = await interactWithTestRunner({
+    'playwright.config.ts': `
+      export default { globalTeardown: './globalTeardown.ts' };
+    `,
+    'globalTeardown.ts': `
+      export default async function() {
+        console.log('teardown1');
+        await new Promise(f => setTimeout(f, 2000));
+        console.log('teardown2');
+      }
+    `,
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('interrupted', async ({ }) => {
+        console.log('\\n%%SEND-SIGINT%%');
+        await new Promise(() => {});
+      });
+    `,
+  });
+  await testProcess.waitForOutput('%%SEND-SIGINT%%');
+  // Send SIGINT twice in quick succession.
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  const { exitCode } = await testProcess.exited;
+  expect(exitCode).toBe(130);
+
+  const result = parseTestRunnerOutput(testProcess.output);
+  expect(result.interrupted).toBe(1);
+  expect(result.output).toContain('teardown1');
+  expect(result.output).toContain('teardown2');
+});
+
+test('slow double SIGINT should be respected', async ({ interactWithTestRunner }) => {
+  test.skip(process.platform === 'win32', 'No sending SIGINT on Windows');
+
+  const testProcess = await interactWithTestRunner({
+    'playwright.config.ts': `
+      export default { globalTeardown: './globalTeardown.ts' };
+    `,
+    'globalTeardown.ts': `
+      export default async function() {
+        console.log('teardown1');
+        await new Promise(f => setTimeout(f, 1000000));
+      }
+    `,
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('interrupted', async ({ }) => {
+        console.log('\\n%%SEND-SIGINT%%');
+        await new Promise(() => {});
+      });
+    `,
+  });
+  await testProcess.waitForOutput('%%SEND-SIGINT%%');
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  await new Promise(f => setTimeout(f, 2000));
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  const { exitCode } = await testProcess.exited;
+  expect(exitCode).toBe(130);
+
+  const result = parseTestRunnerOutput(testProcess.output);
+  expect(result.interrupted).toBe(1);
+  expect(result.output).toContain('teardown1');
+});
+
+test('slow double SIGINT should be respected in reporter.onExit', async ({ interactWithTestRunner }) => {
+  test.skip(process.platform === 'win32', 'No sending SIGINT on Windows');
+
+  const testProcess = await interactWithTestRunner({
+    'playwright.config.ts': `
+      export default { reporter: './reporter' }
+    `,
+    'reporter.ts': `
+      export default class MyReporter {
+        onStdOut(chunk) {
+          process.stdout.write(chunk);
+        }
+
+        async onExit() {
+          // This emulates html reporter, without opening a tab in the default browser.
+          console.log('MyReporter.onExit started');
+          await new Promise(f => setTimeout(f, 100000));
+          console.log('MyReporter.onExit finished');
+        }
+      }
+    `,
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('interrupted', async ({ }) => {
+        console.log('\\n%%SEND-SIGINT%%');
+        await new Promise(() => {});
+      });
+    `,
+  }, { reporter: '' });
+  await testProcess.waitForOutput('%%SEND-SIGINT%%');
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  await new Promise(f => setTimeout(f, 2000));
+  await testProcess.waitForOutput('MyReporter.onExit started');
+  process.kill(testProcess.process.pid!, 'SIGINT');
+  const { exitCode, signal } = await testProcess.exited;
+  expect(exitCode).toBe(null);
+  expect(signal).toBe('SIGINT');  // Default handler should report the signal.
+
+  const result = parseTestRunnerOutput(testProcess.output);
+  expect(result.output).toContain('MyReporter.onExit started');
+  expect(result.output).not.toContain('MyReporter.onExit finished');
+});