[FIX] Improve parallel theme request handling

SAP · Oct 19, 2020 · 88bc0d6 · 88bc0d6
1 parent 0dd503a
commit 88bc0d6
Show file tree

Hide file tree

Showing 2 changed files with 108 additions and 32 deletions.
diff --git a/lib/middleware/serveThemes.js b/lib/middleware/serveThemes.js
@@ -45,6 +45,54 @@ function createMiddleware({resources, middlewareUtil}) {
 	});
 	const buildOptions = {};
 
+	const currentRequests = {};
+
+	async function buildTheme(pathname) {
+		const filename = basename(pathname);
+
+		if (cssVariablesThemeResources.includes(filename) && !buildOptions.cssVariables) {
+			// Activate CSS Variables build the first time a relevant resource is requested
+			buildOptions.cssVariables = true;
+			// Clear the cache to ensure that the build is executed again with "cssVariables: true"
+			builder.clearCache();
+		}
+
+		const sourceLessPath = dirname(pathname) + "/library.source.less";
+		const sourceLessResource = await resources.all.byPath(sourceLessPath);
+		if (!sourceLessResource) { // Not found
+			return;
+		}
+
+		const createdResources = await builder.build([sourceLessResource], buildOptions);
+
+		// Pick requested file resource
+		const resource = createdResources.find((res) => res.getPath().endsWith(filename));
+		if (!resource) {
+			throw new Error(`Theme Build did not return requested file "${pathname}"`);
+		}
+
+		return resource;
+	}
+
+	async function sendResponse(req, res, resource) {
+		const resourcePath = resource.getPath();
+		const {contentType} = middlewareUtil.getMimeInfo(resourcePath);
+		res.setHeader("Content-Type", contentType);
+
+		const content = await resource.getBuffer();
+
+		res.setHeader("ETag", etag(content));
+
+		if (isFresh(req, res)) {
+			// client has a fresh copy of the resource
+			res.statusCode = 304;
+			res.end();
+			return;
+		}
+
+		res.end(content);
+	}
+
 	return async function theme(req, res, next) {
 		try {
 			const pathname = parseurl(req).pathname;
@@ -54,43 +102,18 @@ function createMiddleware({resources, middlewareUtil}) {
 				return;
 			}
 
-			if (cssVariablesThemeResources.includes(filename) && !buildOptions.cssVariables) {
-				// Activate CSS Variables build the first time a relevant resource is requested
-				buildOptions.cssVariables = true;
-				// Clear the cache to ensure that the build is executed again with "cssVariables: true"
-				builder.clearCache();
+			if (!currentRequests[pathname]) {
+				currentRequests[pathname] = buildTheme(pathname);
 			}
 
-			const sourceLessPath = dirname(pathname) + "/library.source.less";
-			const sourceLessResource = await resources.all.byPath(sourceLessPath);
-			if (!sourceLessResource) { // Not found
-				next();
-				return;
-			}
-
-			const createdResources = await builder.build([sourceLessResource], buildOptions);
-			// Pick requested file resource
-			const resource = createdResources.find((res) => res.getPath().endsWith(filename));
+			const resource = await currentRequests[pathname];
 			if (!resource) {
-				next(new Error(`Theme Build did not return requested file "${pathname}"`));
-				return;
-			}
-
-			const resourcePath = resource.getPath();
-			const {contentType} = middlewareUtil.getMimeInfo(resourcePath);
-			res.setHeader("Content-Type", contentType);
-
-			const content = await resource.getBuffer();
-			res.setHeader("ETag", etag(content));
-
-			if (isFresh(req, res)) {
-				// client has a fresh copy of the resource
-				res.statusCode = 304;
-				res.end();
-				return;
+				next();
+			} else {
+				await sendResponse(req, res, resource);
 			}
 
-			res.end(content.toString());
+			delete currentRequests[pathname];
 		} catch (err) {
 			next(err);
 		}

diff --git a/test/lib/server/middleware/serveThemes.js b/test/lib/server/middleware/serveThemes.js
@@ -401,3 +401,56 @@ test.serial.cb("Error handling: Unexpected exception within middleware should ca
 		t.end();
 	});
 });
+
+test.serial("Multiple parallel requests to the same path should only result in one theme build", async (t) => {
+	const resources = createResources();
+
+	const build = stubThemeBuild(resources);
+
+	const {middleware, byPath} = createMiddleware();
+	byPath.withArgs("/resources/sap/ui/test/themes/base/library.source.less")
+		.resolves(resources["library.source.less"]);
+	byPath.withArgs("/resources/sap/ui/test2/themes/base/library.source.less")
+		.resolves(resources["library.source.less"]);
+
+	function request(url) {
+		return new Promise((resolve, reject) => {
+			const req = {
+				url,
+				headers: {}
+			};
+
+			const res = {
+				setHeader: sinon.stub(),
+				getHeader: sinon.stub(),
+				end: resolve
+			};
+
+			middleware(req, res, reject);
+		});
+	}
+
+	await Promise.all([
+		request("/resources/sap/ui/test/themes/base/library.css"),
+		request("/resources/sap/ui/test/themes/base/library.css"),
+		request("/resources/sap/ui/test/themes/base/library.css"),
+
+		request("/resources/sap/ui/test2/themes/base/library.css"),
+		request("/resources/sap/ui/test2/themes/base/library.css")
+	]);
+	// Should only build once per url
+	t.is(build.callCount, 2, "Build should be called 2 times");
+
+
+	// After all requests have finished, the build should be started again when another request comes in
+	await Promise.all([
+		request("/resources/sap/ui/test/themes/base/library.css"),
+		request("/resources/sap/ui/test/themes/base/library.css"),
+		request("/resources/sap/ui/test/themes/base/library.css"),
+
+		request("/resources/sap/ui/test2/themes/base/library.css"),
+		request("/resources/sap/ui/test2/themes/base/library.css")
+	]);
+	// Should only build once per url
+	t.is(build.callCount, 4, "Build should be called 4 times");
+});