metro-file-map: Consistently abort crawl on end() calls

Summary: Changelog: * **[Fix]**: `metro-file-map`: consistently abort crawl when `end()` is called. Ensures that `metro-file-map`'s `build()` method rejects (and terminates early if possible) if `end()` is called either *before* or *during* the `build()` call. Previously this was handled inconsistently. Also switches to a different, more complete `AbortController` polyfill (which is internal and not exposed via the API). Our minimum Node version doesn't support some of the newer methods used here - otherwise we could have used the built-in `AbortController`. NOTE: We can probably do more to bail out of the Node crawler early, similarly to how we shut down the Watchman client immediately on `end()`. Here I'm just setting up the correct API expectations + tests so we can transparently improve the behaviour later. Reviewed By: robhogan Differential Revision: D44104748 fbshipit-source-id: 65c9b897c0f74e421a8d82d16e89d9cb3e48ec9b
facebook · Mar 16, 2023 · 51877a8 · 51877a8
1 parent de19bbd
commit 51877a8
Show file tree

Hide file tree

Showing 10 changed files with 223 additions and 27 deletions.
diff --git a/flow-typed/node-abort-controller.js b/flow-typed/node-abort-controller.js
@@ -0,0 +1,61 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ * @format
+ */
+
+// Translated manually from TS: https://github.com/southpolesteve/node-abort-controller/blob/10e0cea66a069d9319f948d055621e1d37aea5db/index.d.ts
+
+// `AbortSignal`,`AbortController` are defined here to prevent a dependency on the `dom` library which disagrees with node runtime.
+// The definition for `AbortSignal` is taken from @types/node-fetch (https://github.com/DefinitelyTyped/DefinitelyTyped) for
+// maximal compatibility with node-fetch.
+// Original node-fetch definitions are under MIT License.
+
+declare module 'node-abort-controller' {
+  declare export class AbortSignal {
+    aborted: boolean;
+    reason?: any;
+
+    addEventListener: (
+      type: 'abort',
+      listener: (this: AbortSignal, event: any) => any,
+      options?:
+        | boolean
+        | {
+            capture?: boolean,
+            once?: boolean,
+            passive?: boolean,
+          },
+    ) => void;
+
+    removeEventListener: (
+      type: 'abort',
+      listener: (this: AbortSignal, event: any) => any,
+      options?:
+        | boolean
+        | {
+            capture?: boolean,
+          },
+    ) => void;
+
+    dispatchEvent: (event: any) => boolean;
+
+    onabort: null | ((this: AbortSignal, event: any) => void);
+
+    throwIfAborted(): void;
+
+    static abort(reason?: any): AbortSignal;
+
+    static timeout(time: number): AbortSignal;
+  }
+
+  declare export class AbortController {
+    signal: AbortSignal;
+
+    abort(reason?: any): void;
+  }
+}
diff --git a/packages/metro-file-map/package.json b/packages/metro-file-map/package.json
@@ -13,7 +13,6 @@
   },
   "license": "MIT",
   "dependencies": {
-    "abort-controller": "^3.0.0",
     "anymatch": "^3.0.3",
     "debug": "^2.2.0",
     "fb-watchman": "^2.0.0",
@@ -23,6 +22,7 @@
     "jest-util": "^27.2.0",
     "jest-worker": "^27.2.0",
     "micromatch": "^4.0.4",
+    "node-abort-controller": "^3.1.1",
     "nullthrows": "^1.1.1",
     "walker": "^1.0.7"
   },

diff --git a/packages/metro-file-map/src/Watcher.js b/packages/metro-file-map/src/Watcher.js
@@ -18,6 +18,7 @@ import type {
   WatchmanClocks,
 } from './flow-types';
 import type {WatcherOptions as WatcherBackendOptions} from './watchers/common';
+import type {AbortSignal} from 'node-abort-controller';
 
 import watchmanCrawl from './crawlers/watchman';
 import nodeCrawl from './crawlers/node';
@@ -96,6 +97,9 @@ export class Watcher extends EventEmitter {
       path.basename(filePath).startsWith(this._options.healthCheckFilePrefix);
     const crawl = options.useWatchman ? watchmanCrawl : nodeCrawl;
     let crawler = crawl === watchmanCrawl ? 'watchman' : 'node';
+
+    options.abortSignal.throwIfAborted();
+
     const crawlerOptions: CrawlerOptions = {
       abortSignal: options.abortSignal,
       computeSha1: options.computeSha1,

diff --git a/packages/metro-file-map/src/crawlers/__tests__/node-test.js b/packages/metro-file-map/src/crawlers/__tests__/node-test.js
@@ -8,7 +8,7 @@
  * @oncall react_native
  */
 
-'use strict';
+import {AbortController} from 'node-abort-controller';
 
 jest.useRealTimers();
 
@@ -395,4 +395,61 @@ describe('node crawler', () => {
     // once for strawberry.js, once for tomato.js
     expect(fs.lstat).toHaveBeenCalledTimes(2);
   });
+
+  it('aborts the crawl on pre-aborted signal', async () => {
+    nodeCrawl = require('../node');
+    const err = new Error('aborted for test');
+    await expect(
+      nodeCrawl({
+        abortSignal: abortSignalWithReason(err),
+        previousState: {
+          files: new Map(),
+        },
+        extensions: ['js', 'json'],
+        ignore: pearMatcher,
+        rootDir,
+        roots: ['/project/fruits', '/project/vegtables'],
+      }),
+    ).rejects.toThrow(err);
+  });
+
+  it('aborts the crawl if signalled after start', async () => {
+    const err = new Error('aborted for test');
+    const abortController = new AbortController();
+
+    // Pass a fake perf logger that will trigger the abort controller
+    const fakePerfLogger = {
+      point(name, opts) {
+        abortController.abort(err);
+      },
+      annotate() {
+        abortController.abort(err);
+      },
+      subSpan() {
+        return fakePerfLogger;
+      },
+    };
+
+    nodeCrawl = require('../node');
+    await expect(
+      nodeCrawl({
+        perfLogger: fakePerfLogger,
+        abortSignal: abortController.signal,
+        previousState: {
+          files: new Map(),
+        },
+        extensions: ['js', 'json'],
+        ignore: pearMatcher,
+        rootDir,
+        roots: ['/project/fruits', '/project/vegtables'],
+      }),
+    ).rejects.toThrow(err);
+  });
 });
+
+function abortSignalWithReason(reason) {
+  // TODO: use AbortSignal.abort when node-abort-controller supports it
+  const controller = new AbortController();
+  controller.abort(reason);
+  return controller.signal;
+}
diff --git a/packages/metro-file-map/src/crawlers/__tests__/watchman-test.js b/packages/metro-file-map/src/crawlers/__tests__/watchman-test.js
@@ -8,22 +8,30 @@
  * @oncall react_native
  */
 
-'use strict';
+import {AbortController} from 'node-abort-controller';
 
 const path = require('path');
 
 jest.mock('fb-watchman', () => {
   const normalizePathSep = require('../../lib/normalizePathSep').default;
   const Client = jest.fn();
-  Client.prototype.command = jest.fn((args, callback) =>
+  const endedClients = new WeakSet();
+  Client.prototype.command = jest.fn(function (args, callback) {
+    const self = this;
     setImmediate(() => {
+      if (endedClients.has(self)) {
+        callback(new Error('Client has ended'));
+        return;
+      }
       const path = args[1] ? normalizePathSep(args[1]) : undefined;
       const response = mockResponse[args[0]][path];
       callback(null, response.next ? response.next().value : response);
-    }),
-  );
+    });
+  });
   Client.prototype.on = jest.fn();
-  Client.prototype.end = jest.fn();
+  Client.prototype.end = jest.fn(function () {
+    endedClients.add(this);
+  });
   return {Client};
 });
 
@@ -548,4 +556,61 @@ describe('watchman watch', () => {
       }),
     );
   });
+
+  it('aborts the crawl on pre-aborted signal', async () => {
+    const err = new Error('aborted for test');
+    await expect(
+      watchmanCrawl({
+        abortSignal: abortSignalWithReason(err),
+        previousState: {
+          clocks: new Map(),
+          files: new Map(),
+        },
+        extensions: ['js', 'json'],
+        ignore: pearMatcher,
+        rootDir: ROOT_MOCK,
+        roots: ROOTS,
+      }),
+    ).rejects.toThrow(err);
+  });
+
+  it('aborts the crawl if signalled after start', async () => {
+    const err = new Error('aborted for test');
+    const abortController = new AbortController();
+
+    // Pass a fake perf logger that will trigger the abort controller
+    const fakePerfLogger = {
+      point(name, opts) {
+        abortController.abort(err);
+      },
+      annotate() {
+        abortController.abort(err);
+      },
+      subSpan() {
+        return fakePerfLogger;
+      },
+    };
+
+    await expect(
+      watchmanCrawl({
+        perfLogger: fakePerfLogger,
+        abortSignal: abortController.signal,
+        previousState: {
+          clocks: new Map(),
+          files: new Map(),
+        },
+        extensions: ['js', 'json'],
+        ignore: pearMatcher,
+        rootDir: ROOT_MOCK,
+        roots: ROOTS,
+      }),
+    ).rejects.toThrow(err);
+  });
 });
+
+function abortSignalWithReason(reason) {
+  // TODO: use AbortSignal.abort when node-abort-controller supports it
+  const controller = new AbortController();
+  controller.abort(reason);
+  return controller.signal;
+}
diff --git a/packages/metro-file-map/src/crawlers/node/index.js b/packages/metro-file-map/src/crawlers/node/index.js
@@ -175,7 +175,11 @@ module.exports = async function nodeCrawl(options: CrawlerOptions): Promise<{
     includeSymlinks,
     perfLogger,
     roots,
+    abortSignal,
   } = options;
+
+  abortSignal?.throwIfAborted();
+
   perfLogger?.point('nodeCrawl_start');
   const useNativeFind =
     !forceNodeFilesystemAPI &&
@@ -184,7 +188,7 @@ module.exports = async function nodeCrawl(options: CrawlerOptions): Promise<{
 
   debug('Using system find: %s', useNativeFind);
 
-  return new Promise(resolve => {
+  return new Promise((resolve, reject) => {
     const callback = (list: Result) => {
       const changedFiles = new Map<Path, FileMetaData>();
       const removedFiles = new Map(previousState.files);
@@ -208,6 +212,13 @@ module.exports = async function nodeCrawl(options: CrawlerOptions): Promise<{
       }
 
       perfLogger?.point('nodeCrawl_end');
+
+      try {
+        // TODO: Use AbortSignal.reason directly when Flow supports it
+        abortSignal?.throwIfAborted();
+      } catch (e) {
+        reject(e);
+      }
       resolve({
         changedFiles,
         removedFiles,

diff --git a/packages/metro-file-map/src/crawlers/watchman/index.js b/packages/metro-file-map/src/crawlers/watchman/index.js
@@ -62,13 +62,15 @@ module.exports = async function watchmanCrawl({
   removedFiles: FileData,
   clocks: WatchmanClocks,
 }> {
-  perfLogger?.point('watchmanCrawl_start');
-
-  const newClocks = new Map<Path, WatchmanClockSpec>();
+  abortSignal?.throwIfAborted();
 
   const client = new watchman.Client();
   abortSignal?.addEventListener('abort', () => client.end());
 
+  perfLogger?.point('watchmanCrawl_start');
+
+  const newClocks = new Map<Path, WatchmanClockSpec>();
+
   let clientError;
   client.on('error', error => {
     clientError = makeWatchmanError(error);
@@ -280,6 +282,7 @@ module.exports = async function watchmanCrawl({
       });
     }
     perfLogger?.point('watchmanCrawl_end');
+    abortSignal?.throwIfAborted();
     throw (
       queryError ?? clientError ?? new Error('Watchman file results missing')
     );
@@ -378,6 +381,7 @@ module.exports = async function watchmanCrawl({
 
   perfLogger?.point('watchmanCrawl/processResults_end');
   perfLogger?.point('watchmanCrawl_end');
+  abortSignal?.throwIfAborted();
   return {
     changedFiles,
     removedFiles,

diff --git a/packages/metro-file-map/src/flow-types.js b/packages/metro-file-map/src/flow-types.js
@@ -13,6 +13,7 @@
 
 import type ModuleMap from './ModuleMap';
 import type {PerfLoggerFactory, RootPerfLogger, PerfLogger} from 'metro-config';
+import type {AbortSignal} from 'node-abort-controller';
 
 export type {PerfLoggerFactory, PerfLogger};
 

diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js
@@ -56,8 +56,7 @@ import invariant from 'invariant';
 import {escapePathForRegex} from 'jest-regex-util';
 import {Worker} from 'jest-worker';
 import * as path from 'path';
-// $FlowFixMe[untyped-import] - this is a polyfill
-import AbortController from 'abort-controller';
+import {AbortController} from 'node-abort-controller';
 import {performance} from 'perf_hooks';
 import nullthrows from 'nullthrows';
 
@@ -244,7 +243,7 @@ export default class HasteMap extends EventEmitter {
   _watcher: ?Watcher;
   _worker: ?WorkerInterface;
   _cacheManager: CacheManager;
-  _crawlerAbortController: typeof AbortController;
+  _crawlerAbortController: AbortController;
   _healthCheckInterval: ?IntervalID;
   _startupPerfLogger: ?PerfLogger;
 
@@ -1164,11 +1163,12 @@ export default class HasteMap extends EventEmitter {
       clearInterval(this._healthCheckInterval);
     }
 
+    this._crawlerAbortController.abort();
+
     if (!this._watcher) {
       return;
     }
     await this._watcher.close();
-    this._crawlerAbortController.abort();
   }
 
   /**