From 021b313d88dbddf250ac67bfe8320516af9d2036 Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Wed, 30 Nov 2016 03:06:34 -0800 Subject: [PATCH] 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 --- .../react-packager/src/ModuleGraph/Graph.js | 137 +++++++++++++----- .../src/ModuleGraph/__tests__/Graph-test.js | 21 +++ .../src/ModuleGraph/types.flow.js | 7 +- 3 files changed, 123 insertions(+), 42 deletions(-) diff --git a/packager/react-packager/src/ModuleGraph/Graph.js b/packager/react-packager/src/ModuleGraph/Graph.js index 0b2f5533e..fe13718a9 100644 --- a/packager/react-packager/src/ModuleGraph/Graph.js +++ b/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 = { + 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, void | C): void, + resume(): void, + running(): number, + saturated: () => mixed, + started: boolean, + unsaturated: () => mixed, + unshift(T, void | C): void, + workersList(): Array, +}; + +type LoadQueue = + Async$Queue<{id: string, parent: string}, Callback>>; 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) { - const {cwd = '', log = (console: any), optimize = false, skip} = options; + function Graph(entryPoints, platform, options, callback = noop) { + 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 = new Map(); modules.set(null, createParentModule()); - const loadQueue = queue(seq( - ({id, parent}, cb) => resolve(id, parent, platform, options, cb), + const loadQueue: LoadQueue = queue(seq( + ({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)); }; - - function loadModule(id: string, parent: string | null, parentDependencyIndex) { - function onFileLoaded( - error: ?Error, - file: File, - dependencyIDs: Array, - ) { - 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); - } + loadQueue.error = error => { + loadQueue.error = noop; + loadQueue.kill(); + callback(error); + }; 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, + skip?: Set, + cwd: string, +) { + function onFileLoaded( + error?: ?Error, + file?: File, + dependencyIDs?: Array, + ) { + 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; } diff --git a/packager/react-packager/src/ModuleGraph/__tests__/Graph-test.js b/packager/react-packager/src/ModuleGraph/__tests__/Graph-test.js index 2d096b3de..5d468ef64 100644 --- a/packager/react-packager/src/ModuleGraph/__tests__/Graph-test.js +++ b/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'], diff --git a/packager/react-packager/src/ModuleGraph/types.flow.js b/packager/react-packager/src/ModuleGraph/types.flow.js index a1af5edf9..6eb2801c1 100644 --- a/packager/react-packager/src/ModuleGraph/types.flow.js +++ b/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, platform: string, - options?: GraphOptions, + options?: ?GraphOptions, callback?: Callback>, ) => void;