From 041611729a32f9c0ffbeb3a71238bde127c4c1dd Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Wed, 19 May 2021 21:48:57 -0700 Subject: [PATCH] Don't detect async handles already queued to close Some types of async resources in Node.js are not destroyed until *after* their `close` or `end` or similar callbacks and events run, leading to a situation where the `--detectOpenHandles` option can falsely flag resources that have been cleaned up in user code and are already scheduled for destruction. For example, if a test ends from the callback to `TCPServer.close()` and no other tests or lifecycle functions impose additional delays, `--detectOpenHandles` will flag the server even though it has been closed. This is the main cause of issues people encounter with Supertest (see #8554). This addresses the issue by adding a short delay before collecting open handles. Depends on #11382. --- .../__snapshots__/detectOpenHandles.ts.snap | 2 + e2e/__tests__/detectOpenHandles.ts | 12 ++++++ .../__tests__/recently-closed.js | 20 ++++++++++ .../src/__tests__/collectHandles.test.js | 38 +++++++++++++------ packages/jest-core/src/collectHandles.ts | 14 ++++++- packages/jest-core/src/runJest.ts | 6 +-- 6 files changed, 75 insertions(+), 17 deletions(-) create mode 100644 e2e/detect-open-handles/__tests__/recently-closed.js diff --git a/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap b/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap index f679a6e076f5..f4823ed102dd 100644 --- a/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap +++ b/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap @@ -1,5 +1,7 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`does not print info about open handlers for a server that is already closed 1`] = ``; + exports[`prints message about flag on forceExit 1`] = `Force exiting Jest: Have you considered using \`--detectOpenHandles\` to detect async operations that kept running after all tests finished?`; exports[`prints message about flag on slow tests 1`] = ` diff --git a/e2e/__tests__/detectOpenHandles.ts b/e2e/__tests__/detectOpenHandles.ts index f070f4b9e25b..3cd9396f9b26 100644 --- a/e2e/__tests__/detectOpenHandles.ts +++ b/e2e/__tests__/detectOpenHandles.ts @@ -155,3 +155,15 @@ it('prints out info about open handlers from lifecycle functions with a `done` c expect(wrap(textAfterTest)).toMatchSnapshot(); }); + +it('does not print info about open handlers for a server that is already closed', async () => { + const run = runContinuous('detect-open-handles', [ + 'recently-closed', + '--detectOpenHandles', + ]); + await run.waitUntil(({stderr}) => stderr.includes('Ran all test suites')); + const {stderr} = await run.end(); + const textAfterTest = getTextAfterTest(stderr); + + expect(wrap(textAfterTest)).toMatchSnapshot(); +}); diff --git a/e2e/detect-open-handles/__tests__/recently-closed.js b/e2e/detect-open-handles/__tests__/recently-closed.js new file mode 100644 index 000000000000..3ce6b0cfc9ca --- /dev/null +++ b/e2e/detect-open-handles/__tests__/recently-closed.js @@ -0,0 +1,20 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {createServer} from 'http'; + +test('a recently closed server should not be detected by --detectOpenHandles', done => { + const server = createServer((_, response) => response.end('ok')); + server.listen(0, () => { + expect(true).toBe(true); + + // Close server and return immediately on callback. During the "close" + // callback, async hooks usually have not yet been called, but we want to + // make sure Jest can figure out that this server is closed. + server.close(done); + }); +}); diff --git a/packages/jest-core/src/__tests__/collectHandles.test.js b/packages/jest-core/src/__tests__/collectHandles.test.js index 7192709d9350..0970030a5859 100644 --- a/packages/jest-core/src/__tests__/collectHandles.test.js +++ b/packages/jest-core/src/__tests__/collectHandles.test.js @@ -11,12 +11,12 @@ import {PerformanceObserver} from 'perf_hooks'; import collectHandles from '../collectHandles'; describe('collectHandles', () => { - it('should collect Timeout', () => { + it('should collect Timeout', async () => { const handleCollector = collectHandles(); const interval = setInterval(() => {}, 100); - const openHandles = handleCollector(); + const openHandles = await handleCollector(); expect(openHandles).toHaveLength(1); expect(openHandles[0].message).toContain('Timeout'); @@ -24,12 +24,12 @@ describe('collectHandles', () => { clearInterval(interval); }); - it('should not collect the PerformanceObserver open handle', () => { + it('should not collect the PerformanceObserver open handle', async () => { const handleCollector = collectHandles(); const obs = new PerformanceObserver((list, observer) => {}); obs.observe({entryTypes: ['mark']}); - const openHandles = handleCollector(); + const openHandles = await handleCollector(); expect(openHandles).toHaveLength(0); obs.disconnect(); @@ -40,14 +40,28 @@ describe('collectHandles', () => { const server = http.createServer((_, response) => response.end('ok')); server.listen(0, () => { // Collect results while server is still open. - const openHandles = handleCollector(); - - server.close(() => { - expect(openHandles).toContainEqual( - expect.objectContaining({message: 'TCPSERVERWRAP'}), - ); - done(); - }); + handleCollector() + .then(openHandles => { + server.close(() => { + expect(openHandles).toContainEqual( + expect.objectContaining({message: 'TCPSERVERWRAP'}), + ); + done(); + }); + }) + .catch(done); }); }); + + it('should not collect handles that have been queued to close', async () => { + const handleCollector = collectHandles(); + const server = http.createServer((_, response) => response.end('ok')); + + // Start and stop server. + await new Promise(r => server.listen(0, r)); + await new Promise(r => server.close(r)); + + const openHandles = await handleCollector(); + expect(openHandles).toHaveLength(0); + }); }); diff --git a/packages/jest-core/src/collectHandles.ts b/packages/jest-core/src/collectHandles.ts index bb617a3c60bc..6365e2cc0153 100644 --- a/packages/jest-core/src/collectHandles.ts +++ b/packages/jest-core/src/collectHandles.ts @@ -8,12 +8,13 @@ /* eslint-disable local/ban-types-eventually */ import * as asyncHooks from 'async_hooks'; +import {promisify} from 'util'; import stripAnsi = require('strip-ansi'); import type {Config} from '@jest/types'; import {formatExecError} from 'jest-message-util'; import {ErrorWithStack} from 'jest-util'; -export type HandleCollectionResult = () => Array; +export type HandleCollectionResult = () => Promise>; function stackIsFromUser(stack: string) { // Either the test file, or something required by it @@ -42,6 +43,8 @@ const alwaysActive = () => true; // @ts-expect-error: doesn't exist in v10 typings const hasWeakRef = typeof WeakRef === 'function'; +const nextTask = promisify(setImmediate); + // Inspired by https://github.com/mafintosh/why-is-node-running/blob/master/index.js // Extracted as we want to format the result ourselves export default function collectHandles(): HandleCollectionResult { @@ -101,7 +104,14 @@ export default function collectHandles(): HandleCollectionResult { hook.enable(); - return () => { + return async () => { + // Wait until the next JS task for any async resources that have been queued + // for destruction to actually be destroyed. + // For example, Node.js TCP Servers are not destroyed until *after* their + // `close` callback runs. If someone finishes a test from the `close` + // callback, we will not yet have seen the resource be destroyed here. + await nextTask(); + hook.disable(); // Get errors for every async resource still referenced at this moment diff --git a/packages/jest-core/src/runJest.ts b/packages/jest-core/src/runJest.ts index 90d68275ed8c..b6088007235a 100644 --- a/packages/jest-core/src/runJest.ts +++ b/packages/jest-core/src/runJest.ts @@ -75,7 +75,7 @@ type ProcessResultOptions = Pick< outputStream: NodeJS.WriteStream; }; -const processResults = ( +const processResults = async ( runResults: AggregatedResult, options: ProcessResultOptions, ) => { @@ -89,7 +89,7 @@ const processResults = ( } = options; if (collectHandles) { - runResults.openHandles = collectHandles(); + runResults.openHandles = await collectHandles(); } else { runResults.openHandles = []; } @@ -282,7 +282,7 @@ export default async function runJest({ await runGlobalHook({allTests, globalConfig, moduleName: 'globalTeardown'}); } - processResults(results, { + await processResults(results, { collectHandles, json: globalConfig.json, onComplete,