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] Release all proxies of C# and JS objects on WebWorker #88052

Merged
merged 14 commits into from
Jul 24, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 26, 2023

  • all proxies are disposed at the end of the WebWorker lifetime. All proxies would throw if used.
  • internal withInteropCleanupOnExit could turn on proxy disposal at the exit of program, for non MT build too.
    • Added to test-main.js
  • more asserts for thread affinity and disposed objects
  • s_gcHandleFromJSOwnedObject renamed to ThreadJsOwnedObjects and made ThreadStatic for MT
  • dropped locks for ThreadJsOwnedObjects
  • UninstallWebWorkerInterop will now dispose all proxies together with forceDisposeProxies on JS side
  • mono_wasm_pthread_on_pthread_detached will assertNoProxies for all threads
  • added proxy_debug_symbol to all proxies in Debug build of the runtime, for easier diagnostics
  • "OperationCanceledException" throwed on JS side is now new Error not just string.
  • forceDisposeProxies is on INTERNAL and is tested after each method of JSImportExportTest. We also have "keep" property on proxy which will keep it alive anyway, only good for testing.
  • no longer proxy of globalThis can't be disposed from JSHandle.

Fixes #86039
Fixes #88057

@ghost
Copy link

ghost commented Jun 26, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #86039

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 8.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

This comment was marked as resolved.

@pavelsavara

This comment was marked as resolved.

@pavelsavara

This comment was marked as resolved.

@pavelsavara

This comment was marked as resolved.

@lambdageek

This comment was marked as resolved.

@pavelsavara
Copy link
Member Author

I wonder if the runtime already shut down when the timeout callback fired.

Yes, this is on worker after main thread already called mono_exit

@lambdageek
Copy link
Member

Yes, this is on worker after main thread already called mono_exit

Ah ok. does mono_exit actually tell the runtime that things are shutting down? Do we end up calling mono_runtime_try_shutdown or System.Environment.Exit or mono_thread_manage_internal or anything like that?

If it does get called then probably the most straightforward thing is to add if (shutting_down) return; near the beginning of thread_detach.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 4, 2023

does mono_exit actually tell the runtime that things are shutting down?

we call mono_wasm_exit ->mono_jit_cleanup -> mono_thread_manage_internal -> mono_runtime_try_shutdown -> mono_threads_set_shutting_down -> shutting_down = TRUE;

If it does get called then probably the most straightforward thing is to add if (shutting_down) return; near the beginning of thread_detach.

There is mono_gc_thread_detach in thread_detach and I worry that GC could be blocked waiting on the thread to unregister.

@pavelsavara
Copy link
Member Author

we call mono_wasm_exit

No we don't call it. We call mono_exit which is JS only thing.

@lambdageek
Copy link
Member

There is mono_gc_thread_detach in thread_detach and I worry that GC could be blocked waiting on the thread to unregister.

Are you saying you worry that if we exit early from thread_detach then the GC would wait forever for the dead thread?
I don't think that will happen for two reasons:

  1. There shouldn't be any more GCs once we're shutting_down
  2. The GC iterates over MonoThreadInfo* info structures, which is cleaned up by unregister_thread, not by mono_thread_detach_internal. mono_thread_detach_internal is primarily about cleaning up the managed therad object. The GC doesn't look at that one - it should be pretty safe to bypass that stuff if managed code can't be running anymore.

@pavelsavara
Copy link
Member Author

I'm exploring alternate exit strategy here #88387

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 14, 2023

NodeJS strange exit, non-MT

[11:57:48] info: === TEST EXECUTION SUMMARY ===
[11:57:48] info: Total: 446, Errors: 0, Failed: 12, Skipped: 87, Time: 35.1668852s
[11:57:48] info: 
[11:57:48] info: MONO_WASM: {"name":"ExitStatus","message":"Program terminated with exit(1)","status":1}
[11:57:48] info: Process node exited with 1

Log

@pavelsavara pavelsavara added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 14, 2023
# Conflicts:
#	src/mono/wasm/runtime/gc-handles.ts
#	src/mono/wasm/runtime/invoke-cs.ts
#	src/mono/wasm/runtime/loader/exit.ts
#	src/mono/wasm/runtime/loader/run.ts
#	src/mono/wasm/runtime/marshal-to-cs.ts
#	src/mono/wasm/runtime/pthreads/worker/index.ts
#	src/mono/wasm/runtime/snapshot.ts
#	src/mono/wasm/runtime/types/internal.ts
#	src/mono/wasm/test-main.js
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

CI failure is #88900

@pavelsavara
Copy link
Member Author

Example how proxies are disposed on unit tests.
[10:36:43] info: MONO_WASM: forceDisposeProxies done: 19 imports, 0 exports, 7 GCHandles, 2 JSHandles.

[10:36:43] info: Finished:    System.Net.Http.Functional.Tests.dll
[10:36:43] info: 
[10:36:43] info: === TEST EXECUTION SUMMARY ===
[10:36:43] info: Total: 800, Errors: 0, Failed: 0, Skipped: 86, Time: 11.3293999s
[10:36:43] info: 
[10:36:43] info: Received expected 406821 of C:\helix\work\workitem\uploads\xharness-output\testResults.xml
[10:36:43] info: Finished writing 406821 bytes of RESULTXML
[10:36:43] info: Xml file was written to the provided writer.
[10:36:43] info: Tests run: 800 Passed: 714 Inconclusive: 0 Failed: 0 Ignored: 0 Skipped: 86
[10:36:43] info: test-main.js exiting WasmTestRunner.dll System.Net.Http.Functional.Tests.dll with result 0
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 678 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 1142 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 1158 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 1222 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 1302 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# function with GCHandle 566 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of C# object with GCHandle 662 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of JS object with JSHandle 36 was still alive. disposing.
[10:36:43] warn: MONO_WASM: Proxy of JS object with JSHandle 73 was still alive. disposing.
[10:36:43] info: MONO_WASM: forceDisposeProxies done: 19 imports, 0 exports, 7 GCHandles, 2 JSHandles.
[10:36:43] fail: MONO_WASM: Assert failed: mono runtime already exited with 0
[10:36:43] info: WASM EXIT 0

Log

@pavelsavara pavelsavara removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 24, 2023
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 88052 in repo dotnet/runtime

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 24, 2023

Unrelated AOT failure Log

is #89398

[18:12:23] info: Starting:    System.Runtime.CompilerServices.Unsafe.Tests.dll
[18:12:23] info: Error: [MONO] /__w/1/s/src/mono/mono/metadata/object.c:926 <disabled>
[18:12:23] info:     at at (/root/helix/work/workitem/e/wasm_build/AppBundle/_framework/dotnet.runtime.js:3:18041)
[18:12:23] info:     at it (/root/helix/work/workitem/e/wasm_build/AppBundle/_framework/dotnet.runtime.js:3:18298)
[18:12:23] info:     at wasm_trace_logger (wasm_trace_logger (wasm://wasm/0854332a:wasm-function[55016]:0xae04cc))
[18:12:23] info:     at eglib_log_adapter (eglib_log_adapter (wasm://wasm/0854332a:wasm-function[42767]:0x8cc6d1))
[18:12:23] info:     at monoeg_g_logv_nofree (monoeg_g_logv_nofree (wasm://wasm/0854332a:wasm-function[42640]:0x8c80bb))
[18:12:23] info:     at monoeg_g_log (monoeg_g_log (wasm://wasm/0854332a:wasm-function[42642]:0x8c818b))
[18:12:23] info:     at g_log_disabled (g_log_disabled (wasm://wasm/0854332a:wasm-function[42643]:0x8c81c1))
[18:12:23] info:     at compute_class_bitmap (compute_class_bitmap (wasm://wasm/0854332a:wasm-function[45972]:0x975581))
[18:12:23] info:     at mono_class_compute_gc_descriptor (mono_class_compute_gc_descriptor (wasm://wasm/0854332a:wasm-function[45970]:0x9750d5))
[18:12:23] info:     at instantiate_info (instantiate_info (wasm://wasm/0854332a:wasm-function[47390]:0x9bd144))
[18:12:23] info:     at mini_instantiate_gshared_info (mini_instantiate_gshared_info (wasm://wasm/0854332a:wasm-function[47389]:0x9bd002))
[18:12:23] info:     at mini_init_method_rgctx (mini_init_method_rgctx (wasm://wasm/0854332a:wasm-function[47172]:0x9ab48b))
[18:12:23] info:     at aot_instances_System_Runtime_CompilerServices_UnsafeTests__WriteUnaligned_ByRef_StructManagedg__Write_56_0_T_INST_byte__T_INST (aot_instances_System_Runtime_CompilerServices_UnsafeTests__WriteUnaligned_ByRef_StructManagedg__Write_56_0_T_INST_byte__T_INST (wasm://wasm/0854332a:wasm-function[29876]:0x5da7a8))
[18:12:23] info:     at System_Runtime_CompilerServices_Unsafe_Tests_System_Runtime_CompilerServices_UnsafeTests_WriteUnaligned_ByRef_StructManaged (System_Runtime_CompilerServices_Unsafe_Tests_System_Runtime_CompilerServices_UnsafeTests_WriteUnaligned_ByRef_StructManaged (wasm://wasm/0854332a:wasm-f

@pavelsavara
Copy link
Member Author

ENOENT: no such file or directory, open 'http://127.0.0.1:46507/' is #88900, Log

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 24, 2023

Timeout of AOT Microsoft.Extensions.Options.SourceGeneration.Unit.Tests Log

Is #89402

@pavelsavara pavelsavara merged commit 3a88b89 into dotnet:main Jul 24, 2023
114 of 122 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2023
@pavelsavara pavelsavara deleted the browser_mt_cleanup branch September 2, 2024 15:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants