Prevent concurrent builds from happening

src/bundle.ts

@@ -22,10 +22,10 @@ export class Bundle {
 	// Dirty signifies that that the current result is stale, and a new build is
 	// needed. It's reset during the next build.
 	private _dirty = false;
-	// buildsInProgress tracks the number of builds. When a build takes too
-	// long, a new build may have started before the original completed. In this
-	// case, we resolve the old build with the latest result.
-	private buildsInProgress = 0;
+	// buildInProgress tracks the in-progress build. When a build takes too
+	// long, a new build may have been requsted before the original completed.
+	// In this case, we resolve that in-progress build with the pending one.
+	private buildInProgress: Promise<unknown> | null = null;
 	private deferred = new Deferred<BundledFile>();
 	private incrementalBuild: esbuild.BuildIncremental | null = null;
 	private startTime = 0;
@@ -62,18 +62,40 @@ export class Bundle {
 	}
 
 	async write() {
-		if (this.buildsInProgress === 0) this.beforeProcess();
-		this.buildsInProgress++;
-
+		if (this.buildInProgress === null) {
+			this.beforeProcess();
+		} else {
+			// Wait for the previous build to happen before we continue. This prevents
+			// any reentrant behavior, and guarnatees we can get an initial bundle to
+			// create incremental builds from.
+			await this.buildInProgress;
+
+			// There have been multiple calls to write in the time we were
+			// waiting for the in-progress build. Instead of making multiple
+			// calls to rebuild, we resolve with the new in-progress build. One
+			// of the write calls "won" this wait on the in-progress build, and
+			// that winner will eventually resolve the deferred.
+			if (this.buildInProgress !== null) {
+				return this.deferred.promise;
+			}
+		}
 		const { deferred } = this;
-		const result = await this.bundle();
-
-		// The build took so long, we've already had another write.
-		// Instead of serving a stale build, let's wait for the new one to resolve.
-		this.buildsInProgress--;
-		if (this.buildsInProgress > 0 || this._dirty) {
-			deferred.resolve(this.deferred.promise);
-			return deferred.promise;
+		this._dirty = false;
+
+		const build = this.bundle();
+		this.buildInProgress = build;
+		const result = await build;
+		this.buildInProgress = null;
+
+		// The build took so long, we've already had another test file dirty the
+		// bundle. Instead of serving a stale build, let's wait for the new one
+		// to resolve. The new build either hasn't called `write` yet, or it's
+		// waiting in the `await this.buildInProgress` above. Either way, it'll
+		// eventually fire off a new rebuild and resolve the deferred.
+		if (deferred !== this.deferred) {
+			const { promise } = this.deferred;
+			deferred.resolve(promise);
+			return promise;
 		}
 
 		this.afterProcess();
@@ -100,7 +122,10 @@ export class Bundle {
 		// Wait for any in-progress builds to finish. At this point, we know no
 		// new ones will come in, we're just waiting for the current one to
 		// finish running.
-		if (this.buildsInProgress > 0 || this._dirty) {
+		if (this.buildInProgress || this._dirty) {
+			// Wait on the deffered, not the buildInProgress, because the dirty flag
+			// means a new build is imminent. The deferred will only be resolved after
+			// that build is done.
 			await this.deferred.promise;
 		}
 		// Releasing the result allows the child process to end.
@@ -110,15 +135,13 @@ export class Bundle {
 
 	private async bundle() {
 		try {
-			this._dirty = false;
 			if (this.incrementalBuild) {
 				const result = await this.incrementalBuild.rebuild();
 				return this.processResult(result as BuildResult);
 			}
 
 			const result = (await esbuild.build(this.config)) as BuildResult;
 			this.incrementalBuild = result;
-
 			return this.processResult(result);
 		} catch (err) {
 			this.log.error(err.message);

src/utils.ts

@@ -22,6 +22,12 @@ export function debounce<A extends any[], R>(
 	fn: (...args: A) => R,
 	ms: number,
 ) {
+	// This is really just for our tests. Don't do this in your tests, you'll
+	// regret the constant CPU spikes.
+	if (ms < 0) {
+		return fn;
+	}
+
 	let timeout: NodeJS.Timeout;
 	let _args: A | undefined;
 	let _deferred: Deferred<R> | undefined;

test/fixtures/reentrant-initial-bundle/files/main-a.js

@@ -0,0 +1,7 @@
+import { foo } from "./sub/dep1";
+
+describe("simple", () => {
+	it("should work", () => {
+		return foo == 42;
+	});
+});

test/fixtures/reentrant-initial-bundle/files/main-b.js

@@ -0,0 +1,7 @@
+import { foo } from "./sub/dep1";
+
+describe("simple", () => {
+	it("should work", () => {
+		return foo == 42;
+	});
+});

test/fixtures/reentrant-initial-bundle/files/sub/dep1.js

@@ -0,0 +1 @@
+export const foo = 42;

test/fixtures/reentrant-initial-bundle/karma.conf.js

@@ -0,0 +1,37 @@
+const { promises: fs } = require("fs");
+const { baseConfig } = require("../../base.karma.conf");
+
+module.exports = function (config) {
+	let setups = 0;
+	config.set({
+		...baseConfig,
+
+		esbuild: {
+			// Make bundles to happen immediately. In large projects, karma can spend
+			// huge amount of time calculating SHAs of file contents in between calls
+			// to our preprocessor. We need to simulate that "took too long" behavior.
+			bundleDelay: -1,
+
+			plugins: [
+				{
+					name: "delayer",
+
+					setup(build) {
+						if (setups++ > 0) {
+							// We called setup twice! This is likely because we rebuilt before
+							// the initial build was done.
+							throw new Error(`setup #${setups}`);
+						}
+
+						build.onLoad({ filter: /.*/, namespace: "" }, async ({ path }) => {
+							// Insert an arbitrary delay to make the initial build take longer
+							// than bundleDelay.
+							await new Promise(resolve => setTimeout(resolve, 10));
+							return { contents: await fs.readFile(path) };
+						});
+					},
+				},
+			],
+		},
+	});
+};

test/fixtures/reentrant-rebundle/files/main-a.js

@@ -0,0 +1,7 @@
+import { foo } from "./sub/dep1";
+
+describe("simple", () => {
+	it("should work", () => {
+		return foo == 42;
+	});
+});

test/fixtures/reentrant-rebundle/files/main-b.js

@@ -0,0 +1,7 @@
+import { foo } from "./sub/dep1";
+
+describe("simple", () => {
+	it("should work", () => {
+		return foo == 42;
+	});
+});

test/fixtures/reentrant-rebundle/files/sub/dep1.js

@@ -0,0 +1 @@
+export const foo = 42;

test/fixtures/reentrant-rebundle/karma.conf.js

@@ -0,0 +1,33 @@
+const { promises: fs } = require("fs");
+const { baseConfig } = require("../../base.karma.conf");
+const path = require("path");
+
+module.exports = function (config) {
+	let setups = 0;
+	config.set({
+		...baseConfig,
+
+		esbuild: {
+			bundleDelay: -1,
+
+			plugins: [
+				{
+					name: "delayer",
+
+					setup(build) {
+						build.onLoad(
+							{ filter: /.*/, namespace: "" },
+							async ({ path: filePath }) => {
+								console.log(`file: ${path.basename(filePath)}`);
+								// Insert an arbitrary delay to make the build take longer than
+								// bundleDelay.
+								await new Promise(resolve => setTimeout(resolve, 50));
+								return { contents: await fs.readFile(filePath) };
+							},
+						);
+					},
+				},
+			],
+		},
+	});
+};

test/reentrant-initial-bundle.test.ts

@@ -0,0 +1,13 @@
+import { Config } from "pentf/config";
+import { assertEventuallyProgresses, runKarma } from "./test-utils";
+
+export const description = "Rebuild happens before initial build finishes";
+export async function run(config: Config) {
+	const { output } = await runKarma(config, "reentrant-initial-bundle");
+
+	// Both main-*.js tests are necessary, so that we call the preprocessor twice.
+
+	await assertEventuallyProgresses(output.stdout, () => {
+		return output.stdout.some(line => /2 tests completed/.test(line));
+	});
+}

test/reentrant-rebundle.test.ts

@@ -0,0 +1,65 @@
+import { assertEventuallyProgresses, runKarma } from "./test-utils";
+import { promises as fs } from "fs";
+import path from "path";
+import { onTeardown } from "pentf/runner";
+import { strict as assert } from "assert";
+
+export const description =
+	"Reentrant writes after initial build waits for in-progress build to finish, only one pending build may succeed";
+export async function run(config: any) {
+	const { output, resetLog } = await runKarma(config, "reentrant-rebundle");
+
+	// Both main-*.js tests are necessary, so that we call the preprocessor twice.
+
+	await assertEventuallyProgresses(output.stdout, () => {
+		return output.stdout.some(line => /2 tests completed/.test(line));
+	});
+
+	const filePath = path.join(
+		__dirname,
+		"fixtures",
+		"reentrant-rebundle",
+		"files",
+		"sub",
+		"dep1.js",
+	);
+
+	const content = await fs.readFile(filePath, "utf-8");
+	const write = (content: string) => fs.writeFile(filePath, content, "utf-8");
+
+	onTeardown(config, async () => {
+		await write(content);
+	});
+
+	resetLog();
+
+	for (let i = 0; i < 6; i++) {
+		await new Promise(resolve => {
+			const exp = 2 ** i;
+			setTimeout(resolve, 5 * exp);
+		});
+		await write(content);
+	}
+
+	await assertEventuallyProgresses(output.stdout, () => {
+		return output.stdout.some(line => /2 tests completed/.test(line));
+	});
+
+	const files = output.stdout.join("\n").match(/file: .*/g);
+	assert.equal(files?.length, 8);
+
+	// A full build should happen, then the rebuild.
+	const firstBuild = files.splice(0, 4);
+	firstBuild.sort();
+	files.sort();
+
+	// We expect the first build to have loaded all 4 files in the esbuild plugin.
+	// Only then can the second build start.
+	assert.deepEqual(firstBuild, files);
+
+	// Only one build and run should happen,
+	assert.equal(
+		output.stdout.filter(line => /2 tests completed/.test(line)).length,
+		1,
+	);
+}