Permalink
Browse files

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
  • Loading branch information...
davidaurelio authored and Facebook Github Bot committed Nov 30, 2016
1 parent c76f5e1 commit 021b313d88dbddf250ac67bfe8320516af9d2036
@@ -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) {
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,58 +73,76 @@ 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(
({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<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);
}
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.'));
}
}
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;
}
@@ -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'],
@@ -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;

0 comments on commit 021b313

Please sign in to comment.