From 60158e17719db7271445095accc208bfbd389abd Mon Sep 17 00:00:00 2001
From: Dario Piotrowicz <dario@cloudflare.com>
Date: Thu, 18 Jul 2024 23:02:47 +0100
Subject: [PATCH] implement route scoping solution

---
 .changeset/thirty-birds-build.md              | 32 +++++++
 .../dedupeEdgeFunctions.ts                    | 94 ++++++++++++++++++-
 .../templates/_worker.js/index.ts             |  2 +
 .../templates/_worker.js/routesIsolation.ts   | 77 +++++++++++++++
 4 files changed, 200 insertions(+), 5 deletions(-)
 create mode 100644 .changeset/thirty-birds-build.md
 create mode 100644 packages/next-on-pages/templates/_worker.js/routesIsolation.ts

diff --git a/.changeset/thirty-birds-build.md b/.changeset/thirty-birds-build.md
new file mode 100644
index 000000000..54893f1bc
--- /dev/null
+++ b/.changeset/thirty-birds-build.md
@@ -0,0 +1,32 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+fix: implement route specific global scoping strategy
+
+currently routes all share the same global scope, this can be problematic and cause
+race conditions and failures
+
+One example of this is the following code that is present in route function files:
+
+```ts
+self.webpackChunk_N_E = ...
+```
+
+and
+
+```ts
+self.webpackChunk_N_E.push(...)
+```
+
+this indicates that an in-memory global collection of the webpack chunks is shared by all routes,
+this combined with the fact that chunks can have their own module state this can easily cause routes
+to conflict with each other at runtime.
+
+So, in order to solve the above issue wrap every route function in a function wrapper which
+accepts as parameters, thus overrides, the `self`, `globalThis` and `global` symbols. The symbols
+are then to be resolved with proxies that redirect setters to route-scoped in-memory maps and
+getters to the above mentioned map's values and fallback to the original symbol values otherwise
+(i.e. `globalThis` will be overridden by a proxy that, when setting values, sets them in a separate
+location and, when getting values, gets them from said location if present there or from the real
+`globalThis` otherwise)
diff --git a/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts b/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts
index f3295237b..94bc7db44 100644
--- a/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts
+++ b/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts
@@ -143,9 +143,10 @@ async function processFunctionIdentifiers(
 
 		// Build the identifier files before building the function's file.
 		await Promise.all(
-			[...identifierPathsToBuild].map(async path =>
-				buildFile(await readFile(path, 'utf8'), path),
-			),
+			[...identifierPathsToBuild].map(async path => {
+				const fileContents = await functionifyFileContent(path);
+				return buildFile(fileContents, path);
+			}),
 		);
 
 		// If wasm identifier is used in code block, prepend the import to the code block's file.
@@ -168,6 +169,32 @@ async function processFunctionIdentifiers(
 	await Promise.all(functionBuildPromises);
 }
 
+/**
+ * Given a standard ESM file (without imports) it converts it to a function call that returns
+ * an object with the various exports set as its fields
+ *
+ * The function allows us to override global symbols such as `self`, `globalThis` and `global`
+ * (which are used as the function's parameter names)
+ *
+ * @param path the path of the ESM file
+ * @returns the converted file content
+ */
+async function functionifyFileContent(path: string) {
+	let fileContents = await readFile(path, 'utf8');
+	fileContents = `const namedExports = {};${fileContents}`;
+	fileContents = fileContents.replace(
+		/export const (\S+) =/g,
+		'namedExports["$1"] =',
+	);
+	fileContents = `
+		export const getNamedExports = ((self, globalThis, global) => {
+			${fileContents};
+			return namedExports;
+		});
+	`;
+	return fileContents;
+}
+
 /**
  * Builds a new file for an Edge function.
  *
@@ -193,6 +220,9 @@ async function buildFunctionFile(
 		return acc;
 	}, new Map<string, string>());
 
+	let chunkMapIdx = 0;
+	const chunksExportsMap = new Map<string, Set<string>>();
+
 	groupedImports.forEach((keys, path) => {
 		const relativeImportPath = getRelativePathToAncestor({
 			from: newFnLocation,
@@ -202,12 +232,19 @@ async function buildFunctionFile(
 			join(relativeImportPath, addLeadingSlash(path)),
 		);
 
-		functionImports += `import { ${keys} } from '${importPath}';\n`;
+		const namedExportsId = `getNamedExports_${chunkMapIdx++}`;
+		chunksExportsMap.set(namedExportsId, new Set([...keys.split(',')]));
+		functionImports += `import { getNamedExports as ${namedExportsId} } from '${importPath}';\n`;
 	});
 
 	fnInfo.outputPath = relative(workerJsDir, newFnPath);
 
-	const finalFileContents = `${functionImports}${fileContents}`;
+	const finalFileContents = iffefyFunctionFile(
+		fileContents,
+		functionImports,
+		fnInfo,
+		chunksExportsMap,
+	);
 	const buildPromise = buildFile(finalFileContents, newFnPath, {
 		relativeTo: nopDistDir,
 	}).then(async () => {
@@ -225,6 +262,53 @@ type BuildFunctionFileOpts = {
 	newFnPath: string;
 };
 
+/**
+ * Given the content of a function file it converts/wraps it into an iife that overrides the function's contents with an iffe call that
+ * overrides global symbols with route-specific proxies (for more details see: templates/_worker.js/routesIsolation.ts)
+ *
+ * @param fileContents the function file's contents
+ * @param functionImports the imports that need to be added to the file
+ * @param fnInfo the function's information
+ * @param chunksExportsMap a map containing getters and chunks identifiers being used by the function
+ * @returns the updated/iifefied file content
+ */
+function iffefyFunctionFile(
+	fileContents: string,
+	functionImports: string,
+	fnInfo: FunctionInfo,
+	chunksExportsMap: Map<string, Set<string>>,
+): string {
+	const wrappedContent = `
+		export default ((self, globalThis, global) => {
+			${fileContents
+				// it looks like there can be direct references to _ENTRIES (i.e. `_ENTRIES` instead of `globalThis._ENTRIES` etc...)
+				// we have to update all such references otherwise our proxying won't take effect on those
+				.replace(/([^.])_ENTRIES/g, '$1globalThis._ENTRIES')
+				// the default export needs to become the return value of the iife, which is then re-exported as default
+				.replace(/export default /g, 'return ')}
+		})(proxy, proxy, proxy);
+	`;
+
+	const proxyCall = `const proxy = globalThis.__nextOnPagesRoutesIsolation.getProxyFor('${
+		fnInfo.route?.path ?? ''
+	}');`;
+
+	const chunksExtraction = [...chunksExportsMap].flatMap(
+		([getNamedExportsId, keys]) => {
+			return [
+				`const exportsOf${getNamedExportsId} = ${getNamedExportsId}(proxy, proxy, proxy);`,
+				...[...keys].map(
+					key => `const ${key} = exportsOf${getNamedExportsId}["${key}"]`,
+				),
+			];
+		},
+	);
+
+	return [functionImports, proxyCall, ...chunksExtraction, wrappedContent].join(
+		';',
+	);
+}
+
 /**
  * Prepends Wasm imports to a code block's built file.
  *
diff --git a/packages/next-on-pages/templates/_worker.js/index.ts b/packages/next-on-pages/templates/_worker.js/index.ts
index cf29e2d39..50cf212af 100644
--- a/packages/next-on-pages/templates/_worker.js/index.ts
+++ b/packages/next-on-pages/templates/_worker.js/index.ts
@@ -1,5 +1,6 @@
 import { SUSPENSE_CACHE_URL } from '../cache';
 import { handleRequest } from './handleRequest';
+import { setupRoutesIsolation } from './routesIsolation';
 import {
 	adjustRequestForVercel,
 	handleImageResizingRequest,
@@ -22,6 +23,7 @@ declare const __ALSes_PROMISE__: Promise<null | {
 
 export default {
 	async fetch(request, env, ctx) {
+		setupRoutesIsolation();
 		patchFetch();
 
 		const asyncLocalStorages = await __ALSes_PROMISE__;
diff --git a/packages/next-on-pages/templates/_worker.js/routesIsolation.ts b/packages/next-on-pages/templates/_worker.js/routesIsolation.ts
new file mode 100644
index 000000000..a9486603d
--- /dev/null
+++ b/packages/next-on-pages/templates/_worker.js/routesIsolation.ts
@@ -0,0 +1,77 @@
+/**
+ * The next-on-pages worker needs to isolate the global scope for each route, they all sharing the same global scope
+ * allows for race conditions and incorrect behaviors (see: https://github.com/cloudflare/next-on-pages/issues/805)
+ *
+ * So we set up an isolation system in which each route can access a proxy to the global scope that is route-specific,
+ * they can do that by calling `globalThis.getProxyFor(route)`.
+ *
+ * The following function sets up such utility alongside a map that is used to store the various proxies.
+ */
+export function setupRoutesIsolation() {
+	globalThis.__nextOnPagesRoutesIsolation ??= {
+		_map: new Map(),
+		getProxyFor,
+	};
+}
+
+/**
+ * Utility to retrieve a route-specific proxy to the global scope, if the proxy doesn't yet exist it gets created
+ * by the function.
+ *
+ * @param route the target route
+ * @returns the proxy for the route
+ */
+function getProxyFor(route: string) {
+	const existingProxy = globalThis.__nextOnPagesRoutesIsolation._map.get(route);
+	if (existingProxy) {
+		return existingProxy;
+	}
+
+	const newProxy = createNewRouteProxy();
+	globalThis.__nextOnPagesRoutesIsolation._map.set(route, newProxy);
+	return newProxy;
+}
+
+/**
+ * Creates a new route-specific proxy to the global scope.
+ *
+ * How the proxy works: setters on the proxy don't set the values to the actual global scope but
+ * in an internal map specific to the proxy. getters retrieve values from such internal map, and
+ * fall back to the actual global scope for values not present in such map.
+ *
+ * This makes it so that routes trying to modify the global scope will, though this proxy, work
+ * exactly like if they were actually updating the global scope, but without actually doing so,
+ * thus not effecting any other route.
+ *
+ * Note: this does not account for routes trying to update already existing objects in the global
+ * scope (e.g. `globalScope.existing_field.x = 123`), fortunately such granular control doesn't
+ * seem necessary in next-on-pages.
+ *
+ * @returns the created proxy.
+ */
+function createNewRouteProxy() {
+	const overrides = new Map<string | symbol, unknown>();
+
+	return new Proxy(globalThis, {
+		get: (_, property) => {
+			if (overrides.has(property)) {
+				return overrides.get(property);
+			}
+			return Reflect.get(globalThis, property);
+		},
+		set: (_, property, value) => {
+			overrides.set(property, value);
+			return true;
+		},
+	});
+}
+
+type RoutesIsolation = {
+	_map: Map<string, unknown>;
+	getProxyFor: (route: string) => unknown;
+};
+
+declare global {
+	// eslint-disable-next-line no-var
+	var __nextOnPagesRoutesIsolation: RoutesIsolation;
+}