Skip to content

Commit 4c9db8a

Browse files
pavelsavarapavelsavara
authored
[browser][MT] fix thread creation race with finalizer (#100778)
1 parent fadd831 commit 4c9db8a

File tree

14 files changed

+51
-67
lines changed

14 files changed

+51
-67
lines changed

‎src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,17 @@ await executor.Execute(async () =>
591591
}, cts.Token);
592592
}
593593

594+
[Fact]
595+
public async Task FinalizerWorks()
596+
{
597+
var ft = new FinalizerTest();
598+
GC.Collect();
599+
await Task.Delay(100);
600+
GC.Collect();
601+
await Task.Delay(100);
602+
Assert.True(FinalizerTest.FinalizerHit);
603+
}
604+
594605
#endregion
595606

596607
#region Thread Affinity

‎src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,4 +391,13 @@ public class NamedCall
391391

392392
override public string ToString() => Name;
393393
}
394+
395+
public class FinalizerTest
396+
{
397+
public static bool FinalizerHit;
398+
~FinalizerTest()
399+
{
400+
FinalizerHit = true;
401+
}
402+
}
394403
}

‎src/mono/browser/runtime/dotnet.d.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,10 +194,6 @@ type MonoConfig = {
194194
* number of unused workers kept in the emscripten pthread pool after startup
195195
*/
196196
pthreadPoolUnusedSize?: number;
197-
/**
198-
* Delay in milliseconds before starting the finalizer thread
199-
*/
200-
finalizerThreadStartDelayMs?: number;
201197
/**
202198
* If true, a list of the methods optimized by the interpreter will be saved and used for faster startup
203199
* on future runs of the application

‎src/mono/browser/runtime/driver.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -441,9 +441,7 @@ mono_wasm_init_finalizer_thread (void)
441441
{
442442
// in the single threaded build, finalizers periodically run on the main thread instead.
443443
#ifndef DISABLE_THREADS
444-
MONO_ENTER_GC_UNSAFE;
445444
mono_gc_init_finalizer_thread ();
446-
MONO_EXIT_GC_UNSAFE;
447445
#endif
448446
}
449447

‎src/mono/browser/runtime/interp-pgo.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,6 @@ export async function getCacheKey (prefix: string): Promise<string | null> {
198198
delete inputs.logExitCode;
199199
delete inputs.pthreadPoolInitialSize;
200200
delete inputs.pthreadPoolUnusedSize;
201-
delete inputs.finalizerThreadStartDelayMs;
202201
delete inputs.asyncFlushOnExit;
203202
delete inputs.remoteSources;
204203
delete inputs.ignorePdbLoadErrors;

‎src/mono/browser/runtime/loader/assets.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,7 @@ export async function streamingCompileWasm () {
740740
export function preloadWorkers () {
741741
if (!WasmEnableThreads) return;
742742
const jsModuleWorker = resolve_single_asset_path("js-module-threads");
743+
const loadingWorkers = [];
743744
for (let i = 0; i < loaderHelpers.config.pthreadPoolInitialSize!; i++) {
744745
const workerNumber = loaderHelpers.workerNextNumber++;
745746
const worker: Partial<PThreadWorker> = new Worker(jsModuleWorker.resolvedUrl!, {
@@ -753,6 +754,7 @@ export function preloadWorkers () {
753754
threadPrefix: worker_empty_prefix,
754755
threadName: "emscripten-pool",
755756
} as any;
756-
loaderHelpers.loadingWorkers.push(worker as any);
757+
loadingWorkers.push(worker as any);
757758
}
759+
loaderHelpers.loadingWorkers.promise_control.resolve(loadingWorkers);
758760
}

‎src/mono/browser/runtime/loader/config.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,6 @@ export function normalizeConfig () {
194194
if (!Number.isInteger(config.pthreadPoolUnusedSize)) {
195195
config.pthreadPoolUnusedSize = 1;
196196
}
197-
if (!Number.isInteger(config.finalizerThreadStartDelayMs)) {
198-
config.finalizerThreadStartDelayMs = 200;
199-
}
200197
if (config.jsThreadBlockingMode == undefined) {
201198
config.jsThreadBlockingMode = JSThreadBlockingMode.PreventSynchronousJSExport;
202199
}

‎src/mono/browser/runtime/loader/globals.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { exceptions, simd } from "wasm-feature-detect";
88

99
import gitHash from "consts:gitHash";
1010

11-
import type { DotnetModuleInternal, GlobalObjects, LoaderHelpers, MonoConfigInternal, RuntimeHelpers } from "../types/internal";
11+
import type { DotnetModuleInternal, GlobalObjects, LoaderHelpers, MonoConfigInternal, PThreadWorker, RuntimeHelpers } from "../types/internal";
1212
import type { MonoConfig, RuntimeAPI } from "../types";
1313
import { assert_runtime_running, installUnhandledErrorHandler, is_exited, is_runtime_running, mono_exit } from "./exit";
1414
import { assertIsControllablePromise, createPromiseController, getPromiseController } from "./promise-controller";
@@ -95,7 +95,6 @@ export function setLoaderGlobals (
9595
loadedFiles: [],
9696
loadedAssemblies: [],
9797
libraryInitializers: [],
98-
loadingWorkers: [],
9998
workerNextNumber: 1,
10099
actual_downloaded_assets_count: 0,
101100
actual_instantiated_assets_count: 0,
@@ -106,6 +105,7 @@ export function setLoaderGlobals (
106105
allDownloadsQueued: createPromiseController<void>(),
107106
wasmCompilePromise: createPromiseController<WebAssembly.Module>(),
108107
runtimeModuleLoaded: createPromiseController<void>(),
108+
loadingWorkers: createPromiseController<PThreadWorker[]>(),
109109

110110
is_exited,
111111
is_runtime_running,

‎src/mono/browser/runtime/pthreads/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ export {
99
mono_wasm_pthread_ptr, update_thread_info, isMonoThreadMessage, monoThreadInfo,
1010
} from "./shared";
1111
export {
12-
mono_wasm_dump_threads, cancelThreads, is_thread_available,
13-
populateEmscriptenPool, mono_wasm_init_threads, init_finalizer_thread,
12+
mono_wasm_dump_threads, cancelThreads,
13+
populateEmscriptenPool, mono_wasm_init_threads,
1414
waitForThread, replaceEmscriptenPThreadUI
1515
} from "./ui-thread";
1616
export {

‎src/mono/browser/runtime/pthreads/ui-thread.ts

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,7 @@ import { } from "../globals";
88
import { MonoWorkerToMainMessage, monoThreadInfo, mono_wasm_pthread_ptr, update_thread_info, worker_empty_prefix } from "./shared";
99
import { Module, ENVIRONMENT_IS_WORKER, createPromiseController, loaderHelpers, mono_assert, runtimeHelpers } from "../globals";
1010
import { PThreadLibrary, MainToWorkerMessageType, MonoThreadMessage, PThreadInfo, PThreadPtr, PThreadPtrNull, PThreadWorker, PromiseController, Thread, WorkerToMainMessageType, monoMessageSymbol } from "../types/internal";
11-
import { mono_log_error, mono_log_info, mono_log_debug } from "../logging";
12-
import { threads_c_functions as cwraps } from "../cwraps";
11+
import { mono_log_info, mono_log_debug, mono_log_warn } from "../logging";
1312

1413
const threadPromises: Map<PThreadPtr, PromiseController<Thread>[]> = new Map();
1514

@@ -129,13 +128,14 @@ export function onWorkerLoadInitiated (worker: PThreadWorker, loaded: Promise<Wo
129128
}
130129

131130

132-
export function populateEmscriptenPool (): void {
131+
export async function populateEmscriptenPool (): Promise<void> {
133132
if (!WasmEnableThreads) return;
134133
const unused = getUnusedWorkerPool();
135-
for (const worker of loaderHelpers.loadingWorkers) {
134+
const loadingWorkers = await loaderHelpers.loadingWorkers.promise;
135+
for (const worker of loadingWorkers) {
136136
unused.push(worker);
137137
}
138-
loaderHelpers.loadingWorkers = [];
138+
loadingWorkers.length = 0;
139139
}
140140

141141
export async function mono_wasm_init_threads () {
@@ -154,6 +154,8 @@ export async function mono_wasm_init_threads () {
154154
if (workers.length > 0) {
155155
const promises = workers.map(loadWasmModuleToWorker);
156156
await Promise.all(promises);
157+
} else {
158+
mono_log_warn("No workers in the pthread pool, please validate the pthreadPoolInitialSize");
157159
}
158160
}
159161

@@ -202,22 +204,6 @@ export function mono_wasm_dump_threads (): void {
202204
});
203205
}
204206

205-
export function init_finalizer_thread () {
206-
// we don't need it immediately, so we can wait a bit, to keep CPU working on normal startup
207-
setTimeout(() => {
208-
try {
209-
if (loaderHelpers.is_runtime_running()) {
210-
cwraps.mono_wasm_init_finalizer_thread();
211-
} else {
212-
mono_log_debug("init_finalizer_thread skipped");
213-
}
214-
} catch (err) {
215-
mono_log_error("init_finalizer_thread() failed", err);
216-
loaderHelpers.mono_exit(1, err);
217-
}
218-
}, loaderHelpers.config.finalizerThreadStartDelayMs);
219-
}
220-
221207
export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void {
222208
if (!WasmEnableThreads) return;
223209

@@ -226,9 +212,6 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void
226212

227213
modulePThread.loadWasmModuleToWorker = (worker: PThreadWorker): Promise<PThreadWorker> => {
228214
const afterLoaded = originalLoadWasmModuleToWorker(worker);
229-
afterLoaded.then(() => {
230-
availableThreadCount++;
231-
});
232215
onWorkerLoadInitiated(worker, afterLoaded);
233216
if (loaderHelpers.config.exitOnUnhandledError) {
234217
worker.onerror = (e) => {
@@ -258,7 +241,6 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void
258241
}
259242
}));
260243
} else {
261-
availableThreadCount++;
262244
originalReturnWorkerToPool(worker);
263245
}
264246
};
@@ -268,20 +250,13 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void
268250
}
269251
}
270252

271-
let availableThreadCount = 0;
272-
export function is_thread_available () {
273-
if (!WasmEnableThreads) return true;
274-
return availableThreadCount > 0;
275-
}
276-
277253
function getNewWorker (modulePThread: PThreadLibrary): PThreadWorker {
278254
if (!WasmEnableThreads) return null as any;
279255

280256
if (modulePThread.unusedWorkers.length == 0) {
281-
mono_log_debug(`Failed to find unused WebWorker, this may deadlock. Please increase the pthreadPoolReady. Running threads ${modulePThread.runningWorkers.length}. Loading workers: ${modulePThread.unusedWorkers.length}`);
257+
mono_log_debug(`Failed to find unused WebWorker, this may deadlock. Please increase the pthreadPoolInitialSize. Running threads ${modulePThread.runningWorkers.length}. Loading workers: ${modulePThread.unusedWorkers.length}`);
282258
const worker = allocateUnusedWorker();
283259
modulePThread.loadWasmModuleToWorker(worker);
284-
availableThreadCount--;
285260
return worker;
286261
}
287262

@@ -295,12 +270,10 @@ function getNewWorker (modulePThread: PThreadLibrary): PThreadWorker {
295270
const worker = modulePThread.unusedWorkers[i];
296271
if (worker.loaded) {
297272
modulePThread.unusedWorkers.splice(i, 1);
298-
availableThreadCount--;
299273
return worker;
300274
}
301275
}
302-
mono_log_debug(`Failed to find loaded WebWorker, this may deadlock. Please increase the pthreadPoolReady. Running threads ${modulePThread.runningWorkers.length}. Loading workers: ${modulePThread.unusedWorkers.length}`);
303-
availableThreadCount--; // negative value
276+
mono_log_debug(`Failed to find loaded WebWorker, this may deadlock. Please increase the pthreadPoolInitialSize. Running threads ${modulePThread.runningWorkers.length}. Loading workers: ${modulePThread.unusedWorkers.length}`);
304277
return modulePThread.unusedWorkers.pop()!;
305278
}
306279

‎src/mono/browser/runtime/startup.ts

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import { interp_pgo_load_data, interp_pgo_save_data } from "./interp-pgo";
2424
import { mono_log_debug, mono_log_error, mono_log_info, mono_log_warn } from "./logging";
2525

2626
// threads
27-
import { populateEmscriptenPool, mono_wasm_init_threads, init_finalizer_thread } from "./pthreads";
27+
import { populateEmscriptenPool, mono_wasm_init_threads } from "./pthreads";
2828
import { currentWorkerThreadEvents, dotnetPthreadCreated, initWorkerThreadEvents, monoThreadInfo } from "./pthreads";
2929
import { mono_wasm_pthread_ptr, update_thread_info } from "./pthreads";
3030
import { jiterpreter_allocate_tables } from "./jiterpreter-support";
@@ -261,10 +261,6 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void)
261261
FS.chdir(cwd);
262262
}
263263

264-
if (WasmEnableThreads && threadsReady) {
265-
await threadsReady;
266-
}
267-
268264
if (runtimeHelpers.config.interpreterPgo)
269265
setTimeout(maybeSaveInterpPgoTable, (runtimeHelpers.config.interpreterPgoSaveDelay || 15) * 1000);
270266

@@ -275,11 +271,15 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void)
275271
}, 3000);
276272

277273
if (WasmEnableThreads) {
274+
await threadsReady;
275+
278276
// this will create thread and call start_runtime() on it
279277
runtimeHelpers.monoThreadInfo = monoThreadInfo;
280278
runtimeHelpers.isManagedRunningOnCurrentThread = false;
281279
update_thread_info();
282280
runtimeHelpers.managedThreadTID = tcwraps.mono_wasm_create_deputy_thread();
281+
282+
// await mono started on deputy thread
283283
runtimeHelpers.proxyGCHandle = await runtimeHelpers.afterMonoStarted.promise;
284284
runtimeHelpers.ioThreadTID = tcwraps.mono_wasm_create_io_thread();
285285

@@ -292,6 +292,8 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void)
292292
update_thread_info();
293293
bindings_init();
294294

295+
tcwraps.mono_wasm_init_finalizer_thread();
296+
295297
runtimeHelpers.disableManagedTransition = true;
296298
} else {
297299
// load mono runtime and apply environment settings (if necessary)
@@ -411,7 +413,7 @@ async function mono_wasm_pre_init_essential_async (): Promise<void> {
411413
Module.addRunDependency("mono_wasm_pre_init_essential_async");
412414

413415
if (WasmEnableThreads) {
414-
populateEmscriptenPool();
416+
await populateEmscriptenPool();
415417
}
416418

417419
Module.removeRunDependency("mono_wasm_pre_init_essential_async");
@@ -538,9 +540,6 @@ export async function start_runtime () {
538540
update_thread_info();
539541
runtimeHelpers.proxyGCHandle = install_main_synchronization_context(runtimeHelpers.config.jsThreadBlockingMode!);
540542
runtimeHelpers.isManagedRunningOnCurrentThread = true;
541-
542-
// start finalizer thread, lazy
543-
init_finalizer_thread();
544543
}
545544

546545
// get GCHandle of the ctx

‎src/mono/browser/runtime/types/index.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -148,10 +148,6 @@ export type MonoConfig = {
148148
* number of unused workers kept in the emscripten pthread pool after startup
149149
*/
150150
pthreadPoolUnusedSize?: number,
151-
/**
152-
* Delay in milliseconds before starting the finalizer thread
153-
*/
154-
finalizerThreadStartDelayMs?: number,
155151
/**
156152
* If true, a list of the methods optimized by the interpreter will be saved and used for faster startup
157153
* on future runs of the application

‎src/mono/browser/runtime/types/internal.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,6 @@ export type LoaderHelpers = {
131131
scriptUrl: string
132132
modulesUniqueQuery?: string
133133
preferredIcuAsset?: string | null,
134-
loadingWorkers: PThreadWorker[],
135134
workerNextNumber: number,
136135

137136
actual_downloaded_assets_count: number,
@@ -143,6 +142,7 @@ export type LoaderHelpers = {
143142
allDownloadsQueued: PromiseAndController<void>,
144143
wasmCompilePromise: PromiseAndController<WebAssembly.Module>,
145144
runtimeModuleLoaded: PromiseAndController<void>,
145+
loadingWorkers: PromiseAndController<PThreadWorker[]>,
146146

147147
is_exited: () => boolean,
148148
is_runtime_running: () => boolean,

‎src/mono/sample/wasm/browser-threads/Program.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ public partial class Test
1616
private static int _animationCounter = 0;
1717
private static int _callCounter = 0;
1818
private static bool _isRunning = false;
19+
private static Task later;
1920
private static readonly IReadOnlyList<string> _animations = new string[] { "\u2680", "\u2681", "\u2682", "\u2683", "\u2684", "\u2685" };
2021

2122
public static async Task<int> Main(string[] args)
2223
{
2324
Console.WriteLine("Hello, World!");
25+
later = Task.Delay(200); // this will create Timer thread
2426
await updateProgress2();
2527
return 0;
2628
}
@@ -40,12 +42,14 @@ public static void Progress2()
4042
// both calls here are sync POSIX calls dispatched to UI thread, which is already blocked because this is synchronous method on deputy thread
4143
// it should not deadlock anyway, see also invoke_later_when_on_ui_thread_sync and emscripten_yield
4244
var cwd = Directory.GetCurrentDirectory();
43-
Console.WriteLine("Progress! "+ cwd);
45+
Console.WriteLine("Progress2 A " + cwd);
4446

4547
// below is blocking call, which means that UI will spin-lock little longer
4648
// it will warn about blocking wait because of jsThreadBlockingMode: "WarnWhenBlockingWait"
4749
// but it will not deadlock because underlying task chain is not JS promise
48-
Task.Delay(10).Wait();
50+
later.Wait();
51+
52+
Console.WriteLine("Progress2 B");
4953
}
5054

5155
[JSExport]

0 commit comments

Comments
 (0)