Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,4 +391,13 @@ public class NamedCall

override public string ToString() => Name;
}

public class FinalizerTest
{
public static bool FinalizerHit;
~FinalizerTest()
{
FinalizerHit = true;
}
}
}
4 changes: 0 additions & 4 deletions src/mono/browser/runtime/dotnet.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions src/mono/browser/runtime/driver.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
mono_gc_init_finalizer_thread ();
MONO_EXIT_GC_UNSAFE;
#endif
}

Expand Down
1 change: 0 additions & 1 deletion src/mono/browser/runtime/interp-pgo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion src/mono/browser/runtime/loader/assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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!, {
Expand All @@ -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);
}
3 changes: 0 additions & 3 deletions src/mono/browser/runtime/loader/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/mono/browser/runtime/loader/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -95,7 +95,6 @@ export function setLoaderGlobals (
loadedFiles: [],
loadedAssemblies: [],
libraryInitializers: [],
loadingWorkers: [],
workerNextNumber: 1,
actual_downloaded_assets_count: 0,
actual_instantiated_assets_count: 0,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/mono/browser/runtime/pthreads/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
populateEmscriptenPool, mono_wasm_init_threads, init_finalizer_thread,
mono_wasm_dump_threads, cancelThreads,
populateEmscriptenPool, mono_wasm_init_threads,
waitForThread, replaceEmscriptenPThreadUI
} from "./ui-thread";
export {
Expand Down
45 changes: 9 additions & 36 deletions src/mono/browser/runtime/pthreads/ui-thread.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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 () {
Expand All @@ -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");
}
}

Expand Down Expand Up @@ -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;

Expand All @@ -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) => {
Expand Down Expand Up @@ -258,7 +241,6 @@ export function replaceEmscriptenPThreadUI (modulePThread: PThreadLibrary): void
}
}));
} else {
availableThreadCount++;
originalReturnWorkerToPool(worker);
}
};
Expand All @@ -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;
}

Expand All @@ -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()!;
}

Expand Down
17 changes: 8 additions & 9 deletions src/mono/browser/runtime/startup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);

Expand All @@ -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();

Expand All @@ -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)
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions src/mono/browser/runtime/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/mono/browser/runtime/types/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ export type LoaderHelpers = {
scriptUrl: string
modulesUniqueQuery?: string
preferredIcuAsset?: string | null,
loadingWorkers: PThreadWorker[],
workerNextNumber: number,

actual_downloaded_assets_count: number,
Expand All @@ -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,
Expand Down
8 changes: 6 additions & 2 deletions src/mono/sample/wasm/browser-threads/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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]
Expand Down
Loading