[browser][MT] fix thread creation race with finalizer (#100778)

dotnet · Apr 9, 2024 · 4c9db8a · 4c9db8a
1 parent fadd831
commit 4c9db8a
Show file tree

Hide file tree

Showing 14 changed files with 51 additions and 67 deletions.
diff --git a/...pServices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.cs b/...pServices.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/...ces.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTestHelper.cs b/...ces.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
@@ -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
@@ -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
@@ -198,7 +198,6 @@ export async function getCacheKey (prefix: string): Promise<string | null> {
     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
@@ -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<PThreadWorker> = 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
@@ -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
@@ -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<void>(),
         wasmCompilePromise: createPromiseController<WebAssembly.Module>(),
         runtimeModuleLoaded: createPromiseController<void>(),
+        loadingWorkers: createPromiseController<PThreadWorker[]>(),
 
         is_exited,
         is_runtime_running,

diff --git 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
@@ -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<PThreadPtr, PromiseController<Thread>[]> = new Map();
 
@@ -129,13 +128,14 @@ export function onWorkerLoadInitiated (worker: PThreadWorker, loaded: Promise<Wo
 }
 
 
-export function populateEmscriptenPool (): void {
+export async function populateEmscriptenPool (): Promise<void> {
     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<PThreadWorker> => {
         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
@@ -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<void> {
     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
@@ -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
@@ -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<void>,
     wasmCompilePromise: PromiseAndController<WebAssembly.Module>,
     runtimeModuleLoaded: PromiseAndController<void>,
+    loadingWorkers: PromiseAndController<PThreadWorker[]>,
 
     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
@@ -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<string> _animations = new string[] { "\u2680", "\u2681", "\u2682", "\u2683", "\u2684", "\u2685" };
 
     public static async Task<int> 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]