Fix double callback invocation in `ModuleGraph/Graph`

Summary:
The logic in `ModuleGraph/Graph` allowed the callback to be invoked twice, if two invocations of `resolve` call back with errors asynchronously.

This fixes that problem by always calling `queue.kill()` on the asynchronous queue, and only invoke the main callback from the `drain` and `error` queue callbacks.

Reviewed By: jeanlauliac

Differential Revision: D4236797

fbshipit-source-id: c30da7bf7707e13b11270bb2c6117997fd35b029

packager/react-packager/src/ModuleGraph/Graph.js

@@ -12,19 +12,57 @@
 
 const invariant = require('fbjs/lib/invariant');
 const memoize = require('async/memoize');
+const nullthrows = require('fbjs/lib/nullthrows');
 const queue = require('async/queue');
 const seq = require('async/seq');
 
-import type {GraphFn, LoadFn, ResolveFn, File, Module} from './types.flow';
+import type {
+  Callback,
+  File,
+  GraphFn,
+  LoadFn,
+  Module,
+  ResolveFn,
+} from './types.flow';
+
+type Async$Queue<T, C> = {
+  buffer: number,
+  concurrency: number,
+  drain: () => mixed,
+  empty: () => mixed,
+  error: (Error, T) => mixed,
+  idle(): boolean,
+  kill(): void,
+  length(): number,
+  pause(): void,
+  paused: boolean,
+  push(T | Array<T>, void | C): void,
+  resume(): void,
+  running(): number,
+  saturated: () => mixed,
+  started: boolean,
+  unsaturated: () => mixed,
+  unshift(T, void | C): void,
+  workersList(): Array<T>,
+};
+
+type LoadQueue =
+  Async$Queue<{id: string, parent: string}, Callback<File, Array<string>>>;
 
 const createParentModule =
-  () => ({file: {path: '', ast: {}}, dependencies: []});
+  () => ({file: {code: '', isPolyfill: false, path: ''}, dependencies: []});
 
 const noop = () => {};
+const NO_OPTIONS = {};
 
 exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
-  function Graph(entryPoints, platform, options = {}, callback = noop) {
+  function Graph(entryPoints, platform, options, callback = noop) {
-    const {cwd = '', log = (console: any), optimize = false, skip} = options;
+    const {
+      cwd = '',
+      log = (console: any),
+      optimize = false,
+      skip,
+    } = options || NO_OPTIONS;
 
     if (typeof platform !== 'string') {
       log.error('`Graph`, called without a platform');
@@ -35,51 +73,29 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
     const modules: Map<string | null, Module> = new Map();
     modules.set(null, createParentModule());
 
-    const loadQueue = queue(seq(
+    const loadQueue: LoadQueue = queue(seq(
-      ({id, parent}, cb) => resolve(id, parent, platform, options, cb),
+      ({id, parent}, cb) => resolve(id, parent, platform, options || NO_OPTIONS, cb),
       memoize((file, cb) => load(file, {log, optimize}, cb)),
     ), Number.MAX_SAFE_INTEGER);
 
-    const cleanup = () => (loadQueue.drain = noop);
     loadQueue.drain = () => {
-      cleanup();
+      loadQueue.kill();
       callback(null, collect(null, modules));
     };
-
+    loadQueue.error = error => {
-    function loadModule(id: string, parent: string | null, parentDependencyIndex) {
+      loadQueue.error = noop;
-      function onFileLoaded(
+      loadQueue.kill();
-        error: ?Error,
-        file: File,
-        dependencyIDs: Array<string>,
-      ) {
-        if (error) {
-          cleanup();
       callback(error);
-          return;
+    };
-        }
-
-        const parentModule = modules.get(parent);
-        invariant(parentModule, 'Invalid parent module: ' + String(parent));
-        parentModule.dependencies[parentDependencyIndex] = {id, path: file.path};
-
-        if ((!skip || !skip.has(file.path)) && !modules.has(file.path)) {
-          const dependencies = Array(dependencyIDs.length);
-          modules.set(file.path, {file, dependencies});
-          dependencyIDs.forEach(
-            (dependencyID, j) => loadModule(dependencyID, file.path, j));
-        }
-      }
-      loadQueue.push({id, parent: parent != null ? parent : cwd}, onFileLoaded);
-    }
 
     let i = 0;
     for (const entryPoint of entryPoints) {
-      loadModule(entryPoint, null, i++);
+      loadModule(entryPoint, null, i++, loadQueue, modules, skip, cwd, callback);
     }
 
-    if (loadQueue.idle()) {
+    if (i === 0) {
       log.error('`Graph` called without any entry points');
-      cleanup();
+      loadQueue.kill();
       callback(Error('At least one entry point has to be passed.'));
     }
   }
@@ -87,6 +103,46 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
   return Graph;
 };
 
+function loadModule(
+  id: string,
+  parent: string | null,
+  parentDependencyIndex: number,
+  loadQueue: LoadQueue,
+  modules: Map<string | null, Module>,
+  skip?: Set<string>,
+  cwd: string,
+) {
+  function onFileLoaded(
+    error?: ?Error,
+    file?: File,
+    dependencyIDs?: Array<string>,
+  ) {
+    if (error) {
+      return;
+    }
+
+    const {path} = nullthrows(file);
+    dependencyIDs = nullthrows(dependencyIDs);
+
+    const parentModule = modules.get(parent);
+    invariant(parentModule, 'Invalid parent module: ' + String(parent));
+    parentModule.dependencies[parentDependencyIndex] = {id, path};
+
+    if ((!skip || !skip.has(path)) && !modules.has(path)) {
+      const dependencies = Array(dependencyIDs.length);
+      modules.set(path, {dependencies, file: nullthrows(file)});
+      for (let i = 0; i < dependencyIDs.length; ++i) {
+        loadModule(dependencyIDs[i], path, i, loadQueue, modules, skip, cwd);
+      }
+    }
+  }
+
+  loadQueue.push(
+    {id, parent: parent != null ? parent : cwd},
+    onFileLoaded,
+  );
+}
+
 function collect(
   path,
   modules,
@@ -100,8 +156,11 @@ function collect(
     serialized.push(module);
     seen.add(path);
   }
-  module.dependencies.forEach(
+
-    dependency => collect(dependency.path, modules, serialized, seen));
+  const {dependencies} = module;
+  for (var i = 0; i < dependencies.length; i++) {
+    collect(dependencies[i].path, modules, serialized, seen);
+  }
 
   return serialized;
 }

packager/react-packager/src/ModuleGraph/__tests__/Graph-test.js

@@ -10,6 +10,7 @@
 
 jest
   .disableAutomock()
+  .useRealTimers()
   .mock('console');
 
 const {Console} = require('console');
@@ -96,6 +97,26 @@ describe('Graph:', () => {
     });
   });
 
+  it('only calls back once if two parallel invocations of `resolve` fail', done => {
+    load.stub.yields(null, createFile('with two deps'), ['depA', 'depB']);
+    resolve.stub
+      .withArgs('depA').yieldsAsync(new Error())
+      .withArgs('depB').yieldsAsync(new Error());
+
+    let calls = 0;
+    function callback() {
+      if (calls === 0) {
+        process.nextTick(() => {
+          expect(calls).toEqual(1);
+          done();
+        });
+      }
+      ++calls;
+    }
+
+    graph(['entryA', 'entryB'], anyPlatform, noOpts, callback);
+  });
+
   it('passes the files returned by `resolve` on to the `load` function', done => {
     const modules = new Map([
       ['Arbitrary', '/absolute/path/to/Arbitrary.js'],

packager/react-packager/src/ModuleGraph/types.flow.js

@@ -39,8 +39,9 @@ type Dependency = {|
 |};
 
 export type File = {|
-  ast: Object,
+  code: string,
-  code?: string,
+  isPolyfill: boolean,
+  map?: ?Object,
   path: string,
 |};
 
@@ -52,7 +53,7 @@ export type Module = {|
 export type GraphFn = (
   entryPoints: Iterable<string>,
   platform: string,
-  options?: GraphOptions,
+  options?: ?GraphOptions,
   callback?: Callback<Array<Module>>,
 ) => void;