From 4c9db8a94e2033e615c0e4c2ef8d119649b16ba7 Mon Sep 17 00:00:00 2001 From: Pavel Savara Date: Tue, 9 Apr 2024 12:44:58 +0200 Subject: [PATCH] [browser][MT] fix thread creation race with finalizer (#100778) --- .../JavaScript/WebWorkerTest.cs | 11 +++++ .../JavaScript/WebWorkerTestHelper.cs | 9 ++++ src/mono/browser/runtime/dotnet.d.ts | 4 -- src/mono/browser/runtime/driver.c | 2 - src/mono/browser/runtime/interp-pgo.ts | 1 - src/mono/browser/runtime/loader/assets.ts | 4 +- src/mono/browser/runtime/loader/config.ts | 3 -- src/mono/browser/runtime/loader/globals.ts | 4 +- src/mono/browser/runtime/pthreads/index.ts | 4 +- .../browser/runtime/pthreads/ui-thread.ts | 45 ++++--------------- src/mono/browser/runtime/startup.ts | 17 ++++--- src/mono/browser/runtime/types/index.ts | 4 -- src/mono/browser/runtime/types/internal.ts | 2 +- .../sample/wasm/browser-threads/Program.cs | 8 +++- 14 files changed, 51 insertions(+), 67 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs b/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs index 0a2ae44142dc3..24ff04897c2a8 100644 --- a/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs +++ b/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs @@ -591,6 +591,17 @@ await executor.Execute(async () => }, cts.Token); } + [Fact] + public async Task FinalizerWorks() + { + var ft = new FinalizerTest(); + GC.Collect(); + await Task.Delay(100); + GC.Collect(); + await Task.Delay(100); + Assert.True(FinalizerTest.FinalizerHit); + } + #endregion #region Thread Affinity diff --git a/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs b/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs index fa83846f14929..52f3b3a8c387c 100644 --- a/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs +++ b/src/libraries/System.Runtime.InteropServices.JavaScript/tests/System.Runtime.InteropServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs @@ -391,4 +391,13 @@ public class NamedCall override public string ToString() => Name; } + + public class FinalizerTest + { + public static bool FinalizerHit; + ~FinalizerTest() + { + FinalizerHit = true; + } + } } diff --git a/src/mono/browser/runtime/dotnet.d.ts b/src/mono/browser/runtime/dotnet.d.ts index e0d644558b46b..117d6598258a0 100644 --- a/src/mono/browser/runtime/dotnet.d.ts +++ b/src/mono/browser/runtime/dotnet.d.ts @@ -194,10 +194,6 @@ type MonoConfig = { * number of unused workers kept in the emscripten pthread pool after startup */ pthreadPoolUnusedSize?: number; - /** - * Delay in milliseconds before starting the finalizer thread - */ - finalizerThreadStartDelayMs?: number; /** * If true, a list of the methods optimized by the interpreter will be saved and used for faster startup * on future runs of the application diff --git a/src/mono/browser/runtime/driver.c b/src/mono/browser/runtime/driver.c index f231a86119e7e..bca878eb1820d 100644 --- a/src/mono/browser/runtime/driver.c +++ b/src/mono/browser/runtime/driver.c @@ -441,9 +441,7 @@ mono_wasm_init_finalizer_thread (void) { // in the single threaded build, finalizers periodically run on the main thread instead. #ifndef DISABLE_THREADS - MONO_ENTER_GC_UNSAFE; mono_gc_init_finalizer_thread (); - MONO_EXIT_GC_UNSAFE; #endif } diff --git a/src/mono/browser/runtime/interp-pgo.ts b/src/mono/browser/runtime/interp-pgo.ts index 14697385800b1..90557b5f9e810 100644 --- a/src/mono/browser/runtime/interp-pgo.ts +++ b/src/mono/browser/runtime/interp-pgo.ts @@ -198,7 +198,6 @@ export async function getCacheKey (prefix: string): Promise { delete inputs.logExitCode; delete inputs.pthreadPoolInitialSize; delete inputs.pthreadPoolUnusedSize; - delete inputs.finalizerThreadStartDelayMs; delete inputs.asyncFlushOnExit; delete inputs.remoteSources; delete inputs.ignorePdbLoadErrors; diff --git a/src/mono/browser/runtime/loader/assets.ts b/src/mono/browser/runtime/loader/assets.ts index 3ef34458dd839..e657256a7a890 100644 --- a/src/mono/browser/runtime/loader/assets.ts +++ b/src/mono/browser/runtime/loader/assets.ts @@ -740,6 +740,7 @@ export async function streamingCompileWasm () { export function preloadWorkers () { if (!WasmEnableThreads) return; const jsModuleWorker = resolve_single_asset_path("js-module-threads"); + const loadingWorkers = []; for (let i = 0; i < loaderHelpers.config.pthreadPoolInitialSize!; i++) { const workerNumber = loaderHelpers.workerNextNumber++; const worker: Partial = new Worker(jsModuleWorker.resolvedUrl!, { @@ -753,6 +754,7 @@ export function preloadWorkers () { threadPrefix: worker_empty_prefix, threadName: "emscripten-pool", } as any; - loaderHelpers.loadingWorkers.push(worker as any); + loadingWorkers.push(worker as any); } + loaderHelpers.loadingWorkers.promise_control.resolve(loadingWorkers); } diff --git a/src/mono/browser/runtime/loader/config.ts b/src/mono/browser/runtime/loader/config.ts index 5ff185827c106..53abb025deca7 100644 --- a/src/mono/browser/runtime/loader/config.ts +++ b/src/mono/browser/runtime/loader/config.ts @@ -194,9 +194,6 @@ export function normalizeConfig () { if (!Number.isInteger(config.pthreadPoolUnusedSize)) { config.pthreadPoolUnusedSize = 1; } - if (!Number.isInteger(config.finalizerThreadStartDelayMs)) { - config.finalizerThreadStartDelayMs = 200; - } if (config.jsThreadBlockingMode == undefined) { config.jsThreadBlockingMode = JSThreadBlockingMode.PreventSynchronousJSExport; } diff --git a/src/mono/browser/runtime/loader/globals.ts b/src/mono/browser/runtime/loader/globals.ts index 5e76904d55259..b5ce131e7b18f 100644 --- a/src/mono/browser/runtime/loader/globals.ts +++ b/src/mono/browser/runtime/loader/globals.ts @@ -8,7 +8,7 @@ import { exceptions, simd } from "wasm-feature-detect"; import gitHash from "consts:gitHash"; -import type { DotnetModuleInternal, GlobalObjects, LoaderHelpers, MonoConfigInternal, RuntimeHelpers } from "../types/internal"; +import type { DotnetModuleInternal, GlobalObjects, LoaderHelpers, MonoConfigInternal, PThreadWorker, RuntimeHelpers } from "../types/internal"; import type { MonoConfig, RuntimeAPI } from "../types"; import { assert_runtime_running, installUnhandledErrorHandler, is_exited, is_runtime_running, mono_exit } from "./exit"; import { assertIsControllablePromise, createPromiseController, getPromiseController } from "./promise-controller"; @@ -95,7 +95,6 @@ export function setLoaderGlobals ( loadedFiles: [], loadedAssemblies: [], libraryInitializers: [], - loadingWorkers: [], workerNextNumber: 1, actual_downloaded_assets_count: 0, actual_instantiated_assets_count: 0, @@ -106,6 +105,7 @@ export function setLoaderGlobals ( allDownloadsQueued: createPromiseController(), wasmCompilePromise: createPromiseController(), runtimeModuleLoaded: createPromiseController(), + loadingWorkers: createPromiseController(), is_exited, is_runtime_running, diff --git a/src/mono/browser/runtime/pthreads/index.ts b/src/mono/browser/runtime/pthreads/index.ts index 3678d709d1050..25f981d3f3b69 100644 --- a/src/mono/browser/runtime/pthreads/index.ts +++ b/src/mono/browser/runtime/pthreads/index.ts @@ -9,8 +9,8 @@ export { mono_wasm_pthread_ptr, update_thread_info, isMonoThreadMessage, monoThreadInfo, } from "./shared"; export { - mono_wasm_dump_threads, cancelThreads, is_thread_available, - populateEmscriptenPool, mono_wasm_init_threads, init_finalizer_thread, + mono_wasm_dump_threads, cancelThreads, + populateEmscriptenPool, mono_wasm_init_threads, waitForThread, replaceEmscriptenPThreadUI } from "./ui-thread"; export { diff --git a/src/mono/browser/runtime/pthreads/ui-thread.ts b/src/mono/browser/runtime/pthreads/ui-thread.ts index 14a7c9353b2fb..b5f4f72875ec1 100644 --- a/src/mono/browser/runtime/pthreads/ui-thread.ts +++ b/src/mono/browser/runtime/pthreads/ui-thread.ts @@ -8,8 +8,7 @@ import { } from "../globals"; import { MonoWorkerToMainMessage, monoThreadInfo, mono_wasm_pthread_ptr, update_thread_info, worker_empty_prefix } from "./shared"; import { Module, ENVIRONMENT_IS_WORKER, createPromiseController, loaderHelpers, mono_assert, runtimeHelpers } from "../globals"; import { PThreadLibrary, MainToWorkerMessageType, MonoThreadMessage, PThreadInfo, PThreadPtr, PThreadPtrNull, PThreadWorker, PromiseController, Thread, WorkerToMainMessageType, monoMessageSymbol } from "../types/internal"; -import { mono_log_error, mono_log_info, mono_log_debug } from "../logging"; -import { threads_c_functions as cwraps } from "../cwraps"; +import { mono_log_info, mono_log_debug, mono_log_warn } from "../logging"; const threadPromises: Map[]> = new Map(); @@ -129,13 +128,14 @@ export function onWorkerLoadInitiated (worker: PThreadWorker, loaded: Promise { if (!WasmEnableThreads) return; const unused = getUnusedWorkerPool(); - for (const worker of loaderHelpers.loadingWorkers) { + const loadingWorkers = await loaderHelpers.loadingWorkers.promise; + for (const worker of loadingWorkers) { unused.push(worker); } - loaderHelpers.loadingWorkers = []; + loadingWorkers.length = 0; } export async function mono_wasm_init_threads () { @@ -154,6 +154,8 @@ export async function mono_wasm_init_threads () { if (workers.length > 0) { const promises = workers.map(loadWasmModuleToWorker); await Promise.all(promises); + } else { + mono_log_warn("No workers in the pthread pool, please validate the pthreadPoolInitialSize"); } } @@ -202,22 +204,6 @@ export function mono_wasm_dump_threads (): void { }); } -export function init_finalizer_thread () { - // we don't need it immediately, so we can wait a bit, to keep CPU working on normal startup - setTimeout(() => { - try { - if (loaderHelpers.is_runtime_running()) { - cwraps.mono_wasm_init_finalizer_thread(); - } else { - mono_log_debug("init_finalizer_thread skipped"); - } - } catch (err) { - mono_log_error("init_finalizer_thread() failed", err); - loaderHelpers.mono_exit(1, err); - } - }, loaderHelpers.config.finalizerThreadStartDelayMs); -} - export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void { if (!WasmEnableThreads) return; @@ -226,9 +212,6 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void modulePThread.loadWasmModuleToWorker = (worker: PThreadWorker): Promise => { const afterLoaded = originalLoadWasmModuleToWorker(worker); - afterLoaded.then(() => { - availableThreadCount++; - }); onWorkerLoadInitiated(worker, afterLoaded); if (loaderHelpers.config.exitOnUnhandledError) { worker.onerror = (e) => { @@ -258,7 +241,6 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void } })); } else { - availableThreadCount++; originalReturnWorkerToPool(worker); } }; @@ -268,20 +250,13 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void } } -let availableThreadCount = 0; -export function is_thread_available () { - if (!WasmEnableThreads) return true; - return availableThreadCount > 0; -} - function getNewWorker (modulePThread: PThreadLibrary): PThreadWorker { if (!WasmEnableThreads) return null as any; if (modulePThread.unusedWorkers.length == 0) { - 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}`); + 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}`); const worker = allocateUnusedWorker(); modulePThread.loadWasmModuleToWorker(worker); - availableThreadCount--; return worker; } @@ -295,12 +270,10 @@ function getNewWorker (modulePThread: PThreadLibrary): PThreadWorker { const worker = modulePThread.unusedWorkers[i]; if (worker.loaded) { modulePThread.unusedWorkers.splice(i, 1); - availableThreadCount--; return worker; } } - 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}`); - availableThreadCount--; // negative value + 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}`); return modulePThread.unusedWorkers.pop()!; } diff --git a/src/mono/browser/runtime/startup.ts b/src/mono/browser/runtime/startup.ts index c3958289627cf..eeadfa40c83a1 100644 --- a/src/mono/browser/runtime/startup.ts +++ b/src/mono/browser/runtime/startup.ts @@ -24,7 +24,7 @@ import { interp_pgo_load_data, interp_pgo_save_data } from "./interp-pgo"; import { mono_log_debug, mono_log_error, mono_log_info, mono_log_warn } from "./logging"; // threads -import { populateEmscriptenPool, mono_wasm_init_threads, init_finalizer_thread } from "./pthreads"; +import { populateEmscriptenPool, mono_wasm_init_threads } from "./pthreads"; import { currentWorkerThreadEvents, dotnetPthreadCreated, initWorkerThreadEvents, monoThreadInfo } from "./pthreads"; import { mono_wasm_pthread_ptr, update_thread_info } from "./pthreads"; import { jiterpreter_allocate_tables } from "./jiterpreter-support"; @@ -261,10 +261,6 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void) FS.chdir(cwd); } - if (WasmEnableThreads && threadsReady) { - await threadsReady; - } - if (runtimeHelpers.config.interpreterPgo) setTimeout(maybeSaveInterpPgoTable, (runtimeHelpers.config.interpreterPgoSaveDelay || 15) * 1000); @@ -275,11 +271,15 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void) }, 3000); if (WasmEnableThreads) { + await threadsReady; + // this will create thread and call start_runtime() on it runtimeHelpers.monoThreadInfo = monoThreadInfo; runtimeHelpers.isManagedRunningOnCurrentThread = false; update_thread_info(); runtimeHelpers.managedThreadTID = tcwraps.mono_wasm_create_deputy_thread(); + + // await mono started on deputy thread runtimeHelpers.proxyGCHandle = await runtimeHelpers.afterMonoStarted.promise; runtimeHelpers.ioThreadTID = tcwraps.mono_wasm_create_io_thread(); @@ -292,6 +292,8 @@ async function onRuntimeInitializedAsync (userOnRuntimeInitialized: () => void) update_thread_info(); bindings_init(); + tcwraps.mono_wasm_init_finalizer_thread(); + runtimeHelpers.disableManagedTransition = true; } else { // load mono runtime and apply environment settings (if necessary) @@ -411,7 +413,7 @@ async function mono_wasm_pre_init_essential_async (): Promise { Module.addRunDependency("mono_wasm_pre_init_essential_async"); if (WasmEnableThreads) { - populateEmscriptenPool(); + await populateEmscriptenPool(); } Module.removeRunDependency("mono_wasm_pre_init_essential_async"); @@ -538,9 +540,6 @@ export async function start_runtime () { update_thread_info(); runtimeHelpers.proxyGCHandle = install_main_synchronization_context(runtimeHelpers.config.jsThreadBlockingMode!); runtimeHelpers.isManagedRunningOnCurrentThread = true; - - // start finalizer thread, lazy - init_finalizer_thread(); } // get GCHandle of the ctx diff --git a/src/mono/browser/runtime/types/index.ts b/src/mono/browser/runtime/types/index.ts index 73b9f9a2dee63..dad313a47a07e 100644 --- a/src/mono/browser/runtime/types/index.ts +++ b/src/mono/browser/runtime/types/index.ts @@ -148,10 +148,6 @@ export type MonoConfig = { * number of unused workers kept in the emscripten pthread pool after startup */ pthreadPoolUnusedSize?: number, - /** - * Delay in milliseconds before starting the finalizer thread - */ - finalizerThreadStartDelayMs?: number, /** * If true, a list of the methods optimized by the interpreter will be saved and used for faster startup * on future runs of the application diff --git a/src/mono/browser/runtime/types/internal.ts b/src/mono/browser/runtime/types/internal.ts index 0e564f8c87282..cd825edf1ede8 100644 --- a/src/mono/browser/runtime/types/internal.ts +++ b/src/mono/browser/runtime/types/internal.ts @@ -131,7 +131,6 @@ export type LoaderHelpers = { scriptUrl: string modulesUniqueQuery?: string preferredIcuAsset?: string | null, - loadingWorkers: PThreadWorker[], workerNextNumber: number, actual_downloaded_assets_count: number, @@ -143,6 +142,7 @@ export type LoaderHelpers = { allDownloadsQueued: PromiseAndController, wasmCompilePromise: PromiseAndController, runtimeModuleLoaded: PromiseAndController, + loadingWorkers: PromiseAndController, is_exited: () => boolean, is_runtime_running: () => boolean, diff --git a/src/mono/sample/wasm/browser-threads/Program.cs b/src/mono/sample/wasm/browser-threads/Program.cs index 8783ace18be7b..c180b0d80dcd5 100644 --- a/src/mono/sample/wasm/browser-threads/Program.cs +++ b/src/mono/sample/wasm/browser-threads/Program.cs @@ -16,11 +16,13 @@ public partial class Test private static int _animationCounter = 0; private static int _callCounter = 0; private static bool _isRunning = false; + private static Task later; private static readonly IReadOnlyList _animations = new string[] { "\u2680", "\u2681", "\u2682", "\u2683", "\u2684", "\u2685" }; public static async Task Main(string[] args) { Console.WriteLine("Hello, World!"); + later = Task.Delay(200); // this will create Timer thread await updateProgress2(); return 0; } @@ -40,12 +42,14 @@ public static void Progress2() // both calls here are sync POSIX calls dispatched to UI thread, which is already blocked because this is synchronous method on deputy thread // it should not deadlock anyway, see also invoke_later_when_on_ui_thread_sync and emscripten_yield var cwd = Directory.GetCurrentDirectory(); - Console.WriteLine("Progress! "+ cwd); + Console.WriteLine("Progress2 A " + cwd); // below is blocking call, which means that UI will spin-lock little longer // it will warn about blocking wait because of jsThreadBlockingMode: "WarnWhenBlockingWait" // but it will not deadlock because underlying task chain is not JS promise - Task.Delay(10).Wait(); + later.Wait(); + + Console.WriteLine("Progress2 B"); } [JSExport]