Skip to content

Commit 97048f0

Browse files
AbanoubGhadbanAbanoubGhadban
authored
Fix race condition while creating VMs (#527)
1 parent 32fec6b commit 97048f0

File tree

2 files changed

+192
-127
lines changed

2 files changed

+192
-127
lines changed

‎packages/node-renderer/src/worker/vm.ts‎

Lines changed: 151 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ interface VMContext {
3232
// Store contexts by their bundle file paths
3333
const vmContexts = new Map<string, VMContext>();
3434

35+
// Track VM creation promises to handle concurrent buildVM requests
36+
const vmCreationPromises = new Map<string, Promise<boolean>>();
37+
3538
/**
3639
* Returns all bundle paths that have a VM context
3740
*/
@@ -42,7 +45,7 @@ export function hasVMContextForBundle(bundlePath: string) {
4245
/**
4346
* Get a specific VM context by bundle path
4447
*/
45-
function getVMContext(bundlePath: string): VMContext | undefined {
48+
export function getVMContext(bundlePath: string): VMContext | undefined {
4649
return vmContexts.get(bundlePath);
4750
}
4851

@@ -93,6 +96,11 @@ function manageVMPoolSize() {
9396
}
9497

9598
export async function buildVM(filePath: string) {
99+
// Return existing promise if VM is already being created
100+
if (vmCreationPromises.has(filePath)) {
101+
return vmCreationPromises.get(filePath);
102+
}
103+
96104
// Check if VM for this bundle already exists
97105
const vmContext = vmContexts.get(filePath);
98106
if (vmContext) {
@@ -101,142 +109,154 @@ export async function buildVM(filePath: string) {
101109
return Promise.resolve(true);
102110
}
103111

104-
try {
105-
const { supportModules, stubTimers, additionalContext } = getConfig();
106-
const additionalContextIsObject = additionalContext !== null && additionalContext.constructor === Object;
107-
const sharedConsoleHistory = new SharedConsoleHistory();
108-
const contextObject = { sharedConsoleHistory };
109-
110-
if (supportModules) {
111-
// IMPORTANT: When adding anything to this object, update:
112-
// 1. docs/node-renderer/js-configuration.md
113-
// 2. packages/node-renderer/src/shared/configBuilder.ts
114-
extendContext(contextObject, {
115-
Buffer,
116-
TextDecoder,
117-
TextEncoder,
118-
URLSearchParams,
119-
ReadableStream,
120-
process,
121-
setTimeout,
122-
setInterval,
123-
setImmediate,
124-
clearTimeout,
125-
clearInterval,
126-
clearImmediate,
127-
queueMicrotask,
128-
});
129-
}
112+
// Create a new promise for this VM creation
113+
const vmCreationPromise = (async () => {
114+
try {
115+
const { supportModules, stubTimers, additionalContext } = getConfig();
116+
const additionalContextIsObject =
117+
additionalContext !== null && additionalContext.constructor === Object;
118+
const sharedConsoleHistory = new SharedConsoleHistory();
119+
const contextObject = { sharedConsoleHistory };
120+
121+
if (supportModules) {
122+
// IMPORTANT: When adding anything to this object, update:
123+
// 1. docs/node-renderer/js-configuration.md
124+
// 2. packages/node-renderer/src/shared/configBuilder.ts
125+
extendContext(contextObject, {
126+
Buffer,
127+
TextDecoder,
128+
TextEncoder,
129+
URLSearchParams,
130+
ReadableStream,
131+
process,
132+
setTimeout,
133+
setInterval,
134+
setImmediate,
135+
clearTimeout,
136+
clearInterval,
137+
clearImmediate,
138+
queueMicrotask,
139+
});
140+
}
130141

131-
if (additionalContextIsObject) {
132-
extendContext(contextObject, additionalContext);
133-
}
134-
const context = vm.createContext(contextObject);
135-
136-
// Store the new context with timestamp
137-
vmContexts.set(filePath, {
138-
context,
139-
sharedConsoleHistory,
140-
lastUsed: Date.now(),
141-
});
142-
143-
// Manage pool size after adding new VM
144-
manageVMPoolSize();
145-
146-
// Create explicit reference to global context, just in case (some libs can use it):
147-
vm.runInContext('global = this', context);
148-
149-
// Reimplement console methods for replaying on the client:
150-
vm.runInContext(
151-
`
152-
console = {
153-
get history() {
154-
return sharedConsoleHistory.getConsoleHistory();
155-
},
156-
set history(value) {
157-
// Do nothing. It's just for the backward compatibility.
158-
},
159-
};
160-
['error', 'log', 'info', 'warn'].forEach(function (level) {
161-
console[level] = function () {
162-
var argArray = Array.prototype.slice.call(arguments);
163-
if (argArray.length > 0) {
164-
argArray[0] = '[SERVER] ' + argArray[0];
165-
}
166-
sharedConsoleHistory.addToConsoleHistory({level: level, arguments: argArray});
142+
if (additionalContextIsObject) {
143+
extendContext(contextObject, additionalContext);
144+
}
145+
const context = vm.createContext(contextObject);
146+
147+
// Create explicit reference to global context, just in case (some libs can use it):
148+
vm.runInContext('global = this', context);
149+
150+
// Reimplement console methods for replaying on the client:
151+
vm.runInContext(
152+
`
153+
console = {
154+
get history() {
155+
return sharedConsoleHistory.getConsoleHistory();
156+
},
157+
set history(value) {
158+
// Do nothing. It's just for the backward compatibility.
159+
},
167160
};
168-
});`,
169-
context,
170-
);
161+
['error', 'log', 'info', 'warn'].forEach(function (level) {
162+
console[level] = function () {
163+
var argArray = Array.prototype.slice.call(arguments);
164+
if (argArray.length > 0) {
165+
argArray[0] = '[SERVER] ' + argArray[0];
166+
}
167+
sharedConsoleHistory.addToConsoleHistory({level: level, arguments: argArray});
168+
};
169+
});`,
170+
context,
171+
);
172+
173+
// Define global getStackTrace() function:
174+
vm.runInContext(
175+
`
176+
function getStackTrace() {
177+
var stack;
178+
try {
179+
throw new Error('');
180+
}
181+
catch (error) {
182+
stack = error.stack || '';
183+
}
184+
stack = stack.split('\\n').map(function (line) { return line.trim(); });
185+
return stack.splice(stack[0] == 'Error' ? 2 : 1);
186+
}`,
187+
context,
188+
);
171189

172-
// Define global getStackTrace() function:
173-
vm.runInContext(
174-
`
175-
function getStackTrace() {
176-
var stack;
177-
try {
178-
throw new Error('');
190+
if (stubTimers) {
191+
// Define timer polyfills:
192+
vm.runInContext(`function setInterval() {}`, context);
193+
vm.runInContext(`function setTimeout() {}`, context);
194+
vm.runInContext(`function setImmediate() {}`, context);
195+
vm.runInContext(`function clearTimeout() {}`, context);
196+
vm.runInContext(`function clearInterval() {}`, context);
197+
vm.runInContext(`function clearImmediate() {}`, context);
198+
vm.runInContext(`function queueMicrotask() {}`, context);
179199
}
180-
catch (error) {
181-
stack = error.stack || '';
200+
201+
// Run bundle code in created context:
202+
const bundleContents = await readFileAsync(filePath, 'utf8');
203+
204+
// If node-specific code is provided then it must be wrapped into a module wrapper. The bundle
205+
// may need the `require` function, which is not available when running in vm unless passed in.
206+
if (additionalContextIsObject || supportModules) {
207+
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
208+
vm.runInContext(m.wrap(bundleContents), context)(
209+
exports,
210+
require,
211+
module,
212+
filePath,
213+
path.dirname(filePath),
214+
);
215+
} else {
216+
vm.runInContext(bundleContents, context);
182217
}
183-
stack = stack.split('\\n').map(function (line) { return line.trim(); });
184-
return stack.splice(stack[0] == 'Error' ? 2 : 1);
185-
}`,
186-
context,
187-
);
188218

189-
if (stubTimers) {
190-
// Define timer polyfills:
191-
vm.runInContext(`function setInterval() {}`, context);
192-
vm.runInContext(`function setTimeout() {}`, context);
193-
vm.runInContext(`function setImmediate() {}`, context);
194-
vm.runInContext(`function clearTimeout() {}`, context);
195-
vm.runInContext(`function clearInterval() {}`, context);
196-
vm.runInContext(`function clearImmediate() {}`, context);
197-
vm.runInContext(`function queueMicrotask() {}`, context);
198-
}
219+
// Only now, after VM is fully initialized, store the context
220+
vmContexts.set(filePath, {
221+
context,
222+
sharedConsoleHistory,
223+
lastUsed: Date.now(),
224+
});
199225

200-
// Run bundle code in created context:
201-
const bundleContents = await readFileAsync(filePath, 'utf8');
202-
203-
// If node-specific code is provided then it must be wrapped into a module wrapper. The bundle
204-
// may need the `require` function, which is not available when running in vm unless passed in.
205-
if (additionalContextIsObject || supportModules) {
206-
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
207-
vm.runInContext(m.wrap(bundleContents), context)(
208-
exports,
209-
require,
210-
module,
211-
filePath,
212-
path.dirname(filePath),
213-
);
214-
} else {
215-
vm.runInContext(bundleContents, context);
216-
}
226+
// Manage pool size after adding new VM
227+
manageVMPoolSize();
217228

218-
// isWorker check is required for JS unit testing:
219-
if (cluster.isWorker && cluster.worker !== undefined) {
220-
log.debug(`Built VM for worker #${cluster.worker.id} with bundle ${filePath}`);
221-
}
229+
// isWorker check is required for JS unit testing:
230+
if (cluster.isWorker && cluster.worker !== undefined) {
231+
log.debug(`Built VM for worker #${cluster.worker.id} with bundle ${filePath}`);
232+
}
222233

223-
if (log.level === 'debug') {
224-
log.debug(
225-
'Required objects now in VM sandbox context: %s',
226-
vm.runInContext('global.ReactOnRails', context) !== undefined,
227-
);
228-
log.debug(
229-
'Required objects should not leak to the global context (true means OK): %s',
230-
!!global.ReactOnRails,
231-
);
234+
if (log.level === 'debug') {
235+
log.debug(
236+
'Required objects now in VM sandbox context: %s',
237+
vm.runInContext('global.ReactOnRails', context) !== undefined,
238+
);
239+
log.debug(
240+
'Required objects should not leak to the global context (true means OK): %s',
241+
!!global.ReactOnRails,
242+
);
243+
}
244+
245+
return true;
246+
} catch (error) {
247+
log.error('Caught Error when creating context in buildVM, %O', error);
248+
errorReporter.error(error as Error);
249+
throw error;
250+
} finally {
251+
// Always remove the promise from the map when done
252+
vmCreationPromises.delete(filePath);
232253
}
254+
})();
233255

234-
return Promise.resolve(true);
235-
} catch (error) {
236-
log.error('Caught Error when creating context in buildVM, %O', error);
237-
errorReporter.error(error as Error);
238-
return Promise.reject(error as Error);
239-
}
256+
// Store the promise
257+
vmCreationPromises.set(filePath, vmCreationPromise);
258+
259+
return vmCreationPromise;
240260
}
241261

242262
/**
@@ -253,6 +273,11 @@ export async function runInVM(
253273
const { bundlePath } = getConfig();
254274

255275
try {
276+
// Wait for VM creation if it's in progress
277+
if (vmCreationPromises.has(filePath)) {
278+
await vmCreationPromises.get(filePath);
279+
}
280+
256281
// Get the correct VM context based on the provided bundle path
257282
const vmContext = getVMContext(filePath);
258283

‎packages/node-renderer/tests/vm.test.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
createVmBundle,
77
resetForTest,
88
} from './helper';
9-
import { buildVM, hasVMContextForBundle, resetVM, runInVM } from '../src/worker/vm';
9+
import { buildVM, hasVMContextForBundle, resetVM, runInVM, getVMContext } from '../src/worker/vm';
1010
import { getConfig } from '../src/shared/configBuilder';
1111
import { isErrorRenderResult } from '../src/shared/utils';
1212

@@ -523,5 +523,45 @@ describe('buildVM and runInVM', () => {
523523
`console.log.apply(console, [\\"[SERVER] [${otherRequestId}] Console log from Async Server after calling async functions\\"]);`,
524524
);
525525
});
526+
527+
test('calling multiple buildVM in parallel creates the same VM context', async () => {
528+
const buildAndGetVmContext = async () => {
529+
await prepareVM(true);
530+
return getVMContext(serverBundlePath);
531+
};
532+
533+
const [vmContext1, vmContext2] = await Promise.all([buildAndGetVmContext(), buildAndGetVmContext()]);
534+
expect(vmContext1).toBe(vmContext2);
535+
});
536+
537+
test('running runInVM before buildVM', async () => {
538+
resetVM();
539+
void prepareVM(true);
540+
// If the bundle is parsed, ReactOnRails object will be globally available and has the serverRenderReactComponent method
541+
const ReactOnRails = await runInVM(
542+
'typeof ReactOnRails !== "undefined" && ReactOnRails && typeof ReactOnRails.serverRenderReactComponent',
543+
serverBundlePath,
544+
);
545+
expect(ReactOnRails).toEqual('function');
546+
});
547+
548+
test("running multiple buildVM in parallel doesn't cause runInVM to return partial results", async () => {
549+
resetVM();
550+
void Promise.all([prepareVM(true), prepareVM(true), prepareVM(true), prepareVM(true)]);
551+
// If the bundle is parsed, ReactOnRails object will be globally available and has the serverRenderReactComponent method
552+
const runCodeInVM = () =>
553+
runInVM(
554+
'typeof ReactOnRails !== "undefined" && ReactOnRails && typeof ReactOnRails.serverRenderReactComponent',
555+
serverBundlePath,
556+
);
557+
const [runCodeInVM1, runCodeInVM2, runCodeInVM3] = await Promise.all([
558+
runCodeInVM(),
559+
runCodeInVM(),
560+
runCodeInVM(),
561+
]);
562+
expect(runCodeInVM1).toEqual('function');
563+
expect(runCodeInVM2).toEqual('function');
564+
expect(runCodeInVM3).toEqual('function');
565+
});
526566
});
527567
});

0 commit comments

Comments
 (0)