Merge pull request webpack#7027 from webpack/bugfix/load-module-race

sokra · web-flow · commit 40a089c98cff · 2018-04-12T17:09:48.000+02:00
fix race condition when using loadModule
diff --git a/lib/dependencies/LoaderPlugin.js b/lib/dependencies/LoaderPlugin.js
@@ -36,6 +36,7 @@ class LoaderPlugin {
 									}`
 								)
 							);
+						compilation.semaphore.release();
 						compilation.addModuleDependencies(
 							module,
 							[
@@ -48,37 +49,39 @@ class LoaderPlugin {
 							"lm",
 							false,
 							err => {
-								if (err) return callback(err);
+								compilation.semaphore.acquire(() => {
+									if (err) return callback(err);
 
-								if (!dep.module)
-									return callback(new Error("Cannot load the module"));
+									if (!dep.module)
+										return callback(new Error("Cannot load the module"));
 
-								if (dep.module.error) return callback(dep.module.error);
-								if (!dep.module._source)
-									throw new Error(
-										"The module created for a LoaderDependency must have a property _source"
-									);
-								let source, map;
-								const moduleSource = dep.module._source;
-								if (moduleSource.sourceAndMap) {
-									const sourceAndMap = moduleSource.sourceAndMap();
-									map = sourceAndMap.map;
-									source = sourceAndMap.source;
-								} else {
-									map = moduleSource.map();
-									source = moduleSource.source();
-								}
-								if (dep.module.buildInfo.fileDependencies) {
-									for (const d of dep.module.buildInfo.fileDependencies) {
-										loaderContext.addDependency(d);
+									if (dep.module.error) return callback(dep.module.error);
+									if (!dep.module._source)
+										throw new Error(
+											"The module created for a LoaderDependency must have a property _source"
+										);
+									let source, map;
+									const moduleSource = dep.module._source;
+									if (moduleSource.sourceAndMap) {
+										const sourceAndMap = moduleSource.sourceAndMap();
+										map = sourceAndMap.map;
+										source = sourceAndMap.source;
+									} else {
+										map = moduleSource.map();
+										source = moduleSource.source();
 									}
-								}
-								if (dep.module.buildInfo.contextDependencies) {
-									for (const d of dep.module.buildInfo.contextDependencies) {
-										loaderContext.addContextDependency(d);
+									if (dep.module.buildInfo.fileDependencies) {
+										for (const d of dep.module.buildInfo.fileDependencies) {
+											loaderContext.addDependency(d);
+										}
 									}
-								}
-								return callback(null, source, map, dep.module);
+									if (dep.module.buildInfo.contextDependencies) {
+										for (const d of dep.module.buildInfo.contextDependencies) {
+											loaderContext.addContextDependency(d);
+										}
+									}
+									return callback(null, source, map, dep.module);
+								});
 							}
 						);
 					};
diff --git a/lib/util/Semaphore.js b/lib/util/Semaphore.js
@@ -8,6 +8,7 @@ class Semaphore {
 	constructor(available) {
 		this.available = available;
 		this.waiters = [];
+		this._continue = this._continue.bind(this);
 	}
 
 	acquire(callback) {
@@ -20,11 +21,19 @@ class Semaphore {
 	}
 
 	release() {
+		this.available++;
 		if (this.waiters.length > 0) {
-			const callback = this.waiters.pop();
-			process.nextTick(callback);
-		} else {
-			this.available++;
+			process.nextTick(this._continue);
+		}
+	}
+
+	_continue() {
+		if (this.available > 0) {
+			if (this.waiters.length > 0) {
+				this.available--;
+				const callback = this.waiters.pop();
+				callback();
+			}
 		}
 	}
 }
diff --git a/test/configCases/race-conditions/load-module/index.js b/test/configCases/race-conditions/load-module/index.js
@@ -0,0 +1,4 @@
+it("should not deadlock when using loadModule", () => {
+	const result = require("./loader!");
+	result.should.match(/console.log\(42\)/);
+});
diff --git a/test/configCases/race-conditions/load-module/loader.js b/test/configCases/race-conditions/load-module/loader.js
@@ -0,0 +1,15 @@
+module.exports = function() {
+	const callback = this.async();
+	let finished = false;
+	this.loadModule("./module.js", (err, result) => {
+		if (err) return callback(err);
+		if (finished) return;
+		finished = true;
+		callback(null, `module.exports = ${JSON.stringify(result)};`);
+	});
+	setTimeout(() => {
+		if (finished) return;
+		finished = true;
+		callback(new Error("loadModule is hanging"));
+	}, 2000);
+};
diff --git a/test/configCases/race-conditions/load-module/module.js b/test/configCases/race-conditions/load-module/module.js
@@ -0,0 +1 @@
+console.log(42);
diff --git a/test/configCases/race-conditions/load-module/webpack.config.js b/test/configCases/race-conditions/load-module/webpack.config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	parallelism: 1
+};

Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,7 @@ class Semaphore {`
`8`	`8`	`constructor(available) {`
`9`	`9`	`this.available = available;`
`10`	`10`	`this.waiters = [];`
	`11`	`+ this._continue = this._continue.bind(this);`
`11`	`12`	`}`
`12`	`13`
`13`	`14`	`acquire(callback) {`
`@@ -20,11 +21,19 @@ class Semaphore {`
`20`	`21`	`}`
`21`	`22`
`22`	`23`	`release() {`
	`24`	`+ this.available++;`
`23`	`25`	`if (this.waiters.length > 0) {`
`24`		`- const callback = this.waiters.pop();`
`25`		`- process.nextTick(callback);`
`26`		`- } else {`
`27`		`- this.available++;`
	`26`	`+ process.nextTick(this._continue);`
	`27`	`+ }`
	`28`	`+ }`
	`29`	`+`
	`30`	`+ _continue() {`
	`31`	`+ if (this.available > 0) {`
	`32`	`+ if (this.waiters.length > 0) {`
	`33`	`+ this.available--;`
	`34`	`+ const callback = this.waiters.pop();`
	`35`	`+ callback();`
	`36`	`+ }`
`28`	`37`	`}`
`29`	`38`	`}`
`30`	`39`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ parallelism: 1`
	`3`	`+};`