From 4783f9bb73d126000ee02fed5e56d56d5cdd6332 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 11 Apr 2024 13:07:19 -0700 Subject: [PATCH 1/3] skip cleanup of eventpipe for mono to prevent crashes on shutdown --- src/mono/mono/eventpipe/ep-rt-mono.c | 43 ---------------------------- src/mono/mono/eventpipe/ep-rt-mono.h | 5 +++- 2 files changed, 4 insertions(+), 44 deletions(-) diff --git a/src/mono/mono/eventpipe/ep-rt-mono.c b/src/mono/mono/eventpipe/ep-rt-mono.c index 0aa3518df7af77..bed23a49a3eb0d 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.c +++ b/src/mono/mono/eventpipe/ep-rt-mono.c @@ -849,49 +849,6 @@ ep_rt_mono_init_finish (void) mono_error_cleanup (error); } -void -ep_rt_mono_fini (void) -{ - // Avoid cleaning up resources to prevent cleaning up out from under running - // threads. - if (!mono_runtime_is_shutting_down ()) - return; - - ep_rt_mono_runtime_provider_fini (); - ep_rt_mono_profiler_provider_fini (); - - if (_eventpipe_initialized) - mono_rand_close (_rand_provider); - - _rand_provider = NULL; - _eventpipe_initialized = FALSE; - - _ep_rt_mono_runtime_initialized = FALSE; - - if (_ep_rt_mono_default_profiler_provider) { - mono_profiler_set_runtime_initialized_callback (_ep_rt_mono_default_profiler_provider, NULL); - mono_profiler_set_thread_started_callback (_ep_rt_mono_default_profiler_provider, NULL); - mono_profiler_set_thread_stopped_callback (_ep_rt_mono_default_profiler_provider, NULL); - } - _ep_rt_mono_default_profiler_provider = NULL; - - if (_ep_rt_mono_thread_holder_tls_id) - mono_native_tls_free (_ep_rt_mono_thread_holder_tls_id); - _ep_rt_mono_thread_holder_tls_id = 0; - - if (_thread_data_tls_id) - mono_native_tls_free (_thread_data_tls_id); - _thread_data_tls_id = 0; - - _ep_rt_mono_os_cmd_line_init = MONO_LAZY_INIT_STATUS_NOT_INITIALIZED; - _ep_rt_mono_os_cmd_line = NULL; - - _ep_rt_mono_managed_cmd_line_init = MONO_LAZY_INIT_STATUS_NOT_INITIALIZED; - _ep_rt_mono_managed_cmd_line = NULL; - - ep_rt_spin_lock_free (&_ep_rt_mono_config_lock); -} - void EP_CALLBACK_CALLTYPE EventPipeEtwCallbackDotNETRuntimeRundown ( diff --git a/src/mono/mono/eventpipe/ep-rt-mono.h b/src/mono/mono/eventpipe/ep-rt-mono.h index 35a7607681366a..a017df3d93f800 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.h +++ b/src/mono/mono/eventpipe/ep-rt-mono.h @@ -402,7 +402,10 @@ ep_rt_shutdown (void) mono_lazy_cleanup (managed_command_line_get_init (), managed_command_line_lazy_clean); mono_lazy_cleanup (os_command_line_get_init (), os_command_line_lazy_clean); - ep_rt_mono_fini (); + // We were cleaning up resources (mutexes, tls data, etc) here but it races with + // other threads on shutdown. Skipping cleanup to prevent failures. If unloading + // and not leaking these threads becomes a priority we will have to reimplement + // cleanup here. } static From 1b2c9bef5cef6a5483935d9f6b33f9e1493eb919 Mon Sep 17 00:00:00 2001 From: David Mason Date: Thu, 11 Apr 2024 14:48:45 -0700 Subject: [PATCH 2/3] Update ep-rt-mono.h --- src/mono/mono/eventpipe/ep-rt-mono.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/mono/mono/eventpipe/ep-rt-mono.h b/src/mono/mono/eventpipe/ep-rt-mono.h index a017df3d93f800..efc48bc6f99c64 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.h +++ b/src/mono/mono/eventpipe/ep-rt-mono.h @@ -62,7 +62,6 @@ extern void ep_rt_mono_thread_detach (void); extern void ep_rt_mono_component_init (void); extern void ep_rt_mono_init (void); extern void ep_rt_mono_init_finish (void); -extern void ep_rt_mono_fini (void); extern bool ep_rt_mono_walk_managed_stack_for_thread (ep_rt_thread_handle_t thread, EventPipeStackContents *stack_contents); extern bool ep_rt_mono_method_get_simple_assembly_name (ep_rt_method_desc_t *method, ep_char8_t *name, size_t name_len); extern bool ep_rt_mono_method_get_full_name (ep_rt_method_desc_t *method, ep_char8_t *name, size_t name_len); From 752cb87d205f79ce4e9531e6d4dd1ae491724dee Mon Sep 17 00:00:00 2001 From: David Mason Date: Fri, 12 Apr 2024 12:42:00 -0700 Subject: [PATCH 3/3] Code review feedback --- .../eventpipe/ep-rt-mono-profiler-provider.c | 19 +++-------- .../eventpipe/ep-rt-mono-runtime-provider.c | 32 +++---------------- src/mono/mono/eventpipe/ep-rt-mono.c | 18 +++++++++++ src/mono/mono/eventpipe/ep-rt-mono.h | 6 ++-- 4 files changed, 29 insertions(+), 46 deletions(-) diff --git a/src/mono/mono/eventpipe/ep-rt-mono-profiler-provider.c b/src/mono/mono/eventpipe/ep-rt-mono-profiler-provider.c index 2076a2748b6fea..12d730b8455255 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono-profiler-provider.c +++ b/src/mono/mono/eventpipe/ep-rt-mono-profiler-provider.c @@ -3030,9 +3030,6 @@ void ep_rt_mono_profiler_provider_fini (void) { if (_mono_profiler_provider_enabled) { - if (_mono_profiler_provider_callspec.enabled) - mono_callspec_cleanup (&_mono_profiler_provider_callspec); - if (_mono_profiler_provider) { mono_profiler_set_gc_root_register_callback (_mono_profiler_provider, NULL); mono_profiler_set_gc_root_unregister_callback (_mono_profiler_provider, NULL); @@ -3100,7 +3097,6 @@ ep_rt_mono_profiler_provider_fini (void) mono_profiler_set_call_instrumentation_filter_callback (_mono_profiler_provider, NULL); } - _mono_profiler_provider = NULL; if (_mono_heap_dump_profiler_provider) { mono_profiler_set_gc_root_register_callback (_mono_heap_dump_profiler_provider, NULL); @@ -3110,21 +3106,16 @@ ep_rt_mono_profiler_provider_fini (void) mono_profiler_set_gc_resize_callback (_mono_heap_dump_profiler_provider, NULL); mono_profiler_set_gc_finalized_callback (_mono_heap_dump_profiler_provider, NULL); } - _mono_heap_dump_profiler_provider = NULL; _gc_heap_dump_requests = 0; _gc_heap_dump_in_progress = 0; _gc_heap_dump_trigger_count = 0; _gc_state = 0; - - _mono_profiler_provider_enabled = DEFAULT_MONO_PROFILER_PROVIDER_ENABLED; - - gc_heap_dump_mem_block_free_all (); - - gc_heap_dump_request_params_free (); - provider_params_free (); - - ep_rt_spin_lock_free (&_gc_lock); + + // We were cleaning up resources (mutexes, tls data, etc) here but it races with + // other threads on shutdown. Skipping cleanup to prevent failures. If unloading + // and not leaking these threads becomes a priority we will have to reimplement + // cleanup here. } } diff --git a/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c b/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c index a62ddf3aec9bad..ed00c37e1a1faa 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c +++ b/src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c @@ -4630,26 +4630,10 @@ ep_rt_mono_runtime_provider_init (void) void ep_rt_mono_runtime_provider_fini (void) { - if (_sampled_thread_callstacks) - g_array_free (_sampled_thread_callstacks, TRUE); - _sampled_thread_callstacks = NULL; - - _max_sampled_thread_count = 32; - - g_free (_runtime_helper_compile_method_jitinfo); - _runtime_helper_compile_method_jitinfo = NULL; - - if (_runtime_helper_compile_method) - mono_free_method (_runtime_helper_compile_method); - _runtime_helper_compile_method = NULL; - - g_free (_monitor_enter_method_jitinfo); - _monitor_enter_method_jitinfo = NULL; - _monitor_enter_method = NULL; - - g_free (_monitor_enter_v4_method_jitinfo); - _monitor_enter_v4_method_jitinfo = NULL; - _monitor_enter_v4_method = NULL; + // We were cleaning up resources (mutexes, tls data, etc) here but it races with + // other threads on shutdown. Skipping cleanup to prevent failures. If unloading + // and not leaking these threads becomes a priority we will have to reimplement + // cleanup here. if (_ep_rt_mono_default_profiler_provider) { mono_profiler_set_jit_begin_callback (_ep_rt_mono_default_profiler_provider, NULL); @@ -4680,14 +4664,6 @@ ep_rt_mono_runtime_provider_fini (void) _gc_heap_dump_requests = 0; _gc_heap_dump_count = 0; _gc_heap_dump_trigger_count = 0; - - dn_vector_dispose (&_gc_heap_dump_requests_data); - memset (&_gc_heap_dump_requests_data, 0, sizeof (_gc_heap_dump_requests_data)); - - dn_umap_dispose (&_gc_roots_table); - memset (&_gc_roots_table, 0, sizeof (_gc_roots_table)); - - ep_rt_spin_lock_free (&_gc_lock); } void diff --git a/src/mono/mono/eventpipe/ep-rt-mono.c b/src/mono/mono/eventpipe/ep-rt-mono.c index bed23a49a3eb0d..a8890ca4bf66e6 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.c +++ b/src/mono/mono/eventpipe/ep-rt-mono.c @@ -849,6 +849,24 @@ ep_rt_mono_init_finish (void) mono_error_cleanup (error); } +void +ep_rt_mono_fini (void) +{ + ep_rt_mono_runtime_provider_fini (); + ep_rt_mono_profiler_provider_fini (); + + if (_ep_rt_mono_default_profiler_provider) { + mono_profiler_set_runtime_initialized_callback (_ep_rt_mono_default_profiler_provider, NULL); + mono_profiler_set_thread_started_callback (_ep_rt_mono_default_profiler_provider, NULL); + mono_profiler_set_thread_stopped_callback (_ep_rt_mono_default_profiler_provider, NULL); + } + + // We were cleaning up resources (mutexes, tls data, etc) here but it races with + // other threads on shutdown. Skipping cleanup to prevent failures. If unloading + // and not leaking these threads becomes a priority we will have to reimplement + // cleanup here. +} + void EP_CALLBACK_CALLTYPE EventPipeEtwCallbackDotNETRuntimeRundown ( diff --git a/src/mono/mono/eventpipe/ep-rt-mono.h b/src/mono/mono/eventpipe/ep-rt-mono.h index efc48bc6f99c64..35a7607681366a 100644 --- a/src/mono/mono/eventpipe/ep-rt-mono.h +++ b/src/mono/mono/eventpipe/ep-rt-mono.h @@ -62,6 +62,7 @@ extern void ep_rt_mono_thread_detach (void); extern void ep_rt_mono_component_init (void); extern void ep_rt_mono_init (void); extern void ep_rt_mono_init_finish (void); +extern void ep_rt_mono_fini (void); extern bool ep_rt_mono_walk_managed_stack_for_thread (ep_rt_thread_handle_t thread, EventPipeStackContents *stack_contents); extern bool ep_rt_mono_method_get_simple_assembly_name (ep_rt_method_desc_t *method, ep_char8_t *name, size_t name_len); extern bool ep_rt_mono_method_get_full_name (ep_rt_method_desc_t *method, ep_char8_t *name, size_t name_len); @@ -401,10 +402,7 @@ ep_rt_shutdown (void) mono_lazy_cleanup (managed_command_line_get_init (), managed_command_line_lazy_clean); mono_lazy_cleanup (os_command_line_get_init (), os_command_line_lazy_clean); - // We were cleaning up resources (mutexes, tls data, etc) here but it races with - // other threads on shutdown. Skipping cleanup to prevent failures. If unloading - // and not leaking these threads becomes a priority we will have to reimplement - // cleanup here. + ep_rt_mono_fini (); } static