From fdf7ff4d2f7dd8f7c5911eb9f5256495f0b40c67 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 14:54:47 -0400 Subject: [PATCH 1/7] [EventPipe] Add provider callback completion fields --- src/native/eventpipe/ep-provider.c | 5 +++++ src/native/eventpipe/ep-provider.h | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 5f997608ef80ac..1467e6336fa864 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -193,6 +193,9 @@ ep_provider_alloc ( instance->event_list = dn_list_alloc (); ep_raise_error_if_nok (instance->event_list != NULL); + ep_rt_wait_event_alloc (&instance->callbacks_complete_event, true /* bManual */, false /* bInitial */); + ep_raise_error_if_nok (ep_rt_wait_event_is_valid (&instance->callbacks_complete_event)); + instance->keywords = 0; instance->provider_level = EP_EVENT_LEVEL_CRITICAL; instance->callback_func = callback_func; @@ -200,6 +203,7 @@ ep_provider_alloc ( instance->config = config; instance->delete_deferred = false; instance->sessions = 0; + instance->uninvoked_prepared_callbacks_counter = 0; ep_on_exit: return instance; @@ -225,6 +229,7 @@ ep_provider_free (EventPipeProvider * provider) } ep_on_exit: + ep_rt_wait_event_free (&provider->callbacks_complete_event); ep_rt_utf16_string_free (provider->provider_name_utf16); ep_rt_utf8_string_free (provider->provider_name); ep_rt_object_free (provider); diff --git a/src/native/eventpipe/ep-provider.h b/src/native/eventpipe/ep-provider.h index abb743f72d8fcb..85cb7bc6cc1a35 100644 --- a/src/native/eventpipe/ep-provider.h +++ b/src/native/eventpipe/ep-provider.h @@ -42,6 +42,13 @@ struct _EventPipeProvider_Internal { // True if the provider has been deleted, but that deletion // has been deferred until tracing is stopped. bool delete_deferred; + // The number of EventPipeProvider callbacks that have been + // prepared but not yet invoked. + // Used to determine when it is safe to delete a provider. + int64_t uninvoked_prepared_callbacks_counter; + // Event object used to signal eventpipe provider deletion + // that all in flight callbacks have completed. + ep_rt_wait_event_handle_t callbacks_complete_event; }; #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_PROVIDER_GETTER_SETTER) From f1addc0867acf886d06f8a86cd0039711ccba629 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 15:04:59 -0400 Subject: [PATCH 2/7] [EventPipe] Add provider field to CallbackData Give the callback data access to the associated provider so it can decrement the provider's callbacks counter after the callback invocation is completed. --- .../test/ep-provider-callback-dataqueue-tests.c | 3 ++- src/native/eventpipe/ep-provider.c | 3 ++- src/native/eventpipe/ep-types.h | 7 +++++-- src/native/eventpipe/ep.c | 10 +++++++--- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c b/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c index 32f7c7d00038ce..309d5a60a8f994 100644 --- a/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c +++ b/src/mono/mono/eventpipe/test/ep-provider-callback-dataqueue-tests.c @@ -57,7 +57,8 @@ test_provider_callback_data_queue (void) 1, EP_EVENT_LEVEL_LOGALWAYS, true, - 0); + 0, + NULL); ep_provider_callback_data_queue_enqueue (provider_callback_data_queue, provider_enqueue_callback_data); ep_provider_callback_data_fini (provider_enqueue_callback_data); } diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 1467e6336fa864..5fdba63fe1f549 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -86,7 +86,8 @@ provider_prepare_callback_data ( keywords, provider_level, provider->sessions != 0, - session_id); + session_id, + provider); } static diff --git a/src/native/eventpipe/ep-types.h b/src/native/eventpipe/ep-types.h index 5273f2b5ac5787..79b05ea8bc6718 100644 --- a/src/native/eventpipe/ep-types.h +++ b/src/native/eventpipe/ep-types.h @@ -76,6 +76,7 @@ struct _EventPipeProviderCallbackData_Internal { EventPipeEventLevel provider_level; bool enabled; EventPipeSessionID session_id; + EventPipeProvider *provider; }; #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_EP_GETTER_SETTER) @@ -100,7 +101,8 @@ ep_provider_callback_data_alloc ( int64_t keywords, EventPipeEventLevel provider_level, bool enabled, - EventPipeSessionID session_id); + EventPipeSessionID session_id, + EventPipeProvider *provider); EventPipeProviderCallbackData * ep_provider_callback_data_alloc_copy (EventPipeProviderCallbackData *provider_callback_data_src); @@ -117,7 +119,8 @@ ep_provider_callback_data_init ( int64_t keywords, EventPipeEventLevel provider_level, bool enabled, - EventPipeSessionID session_id); + EventPipeSessionID session_id, + EventPipeProvider *provider); EventPipeProviderCallbackData * ep_provider_callback_data_init_copy ( diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c index b65326a16bcfa1..5b9dcc557266f5 100644 --- a/src/native/eventpipe/ep.c +++ b/src/native/eventpipe/ep.c @@ -225,7 +225,8 @@ ep_provider_callback_data_alloc ( int64_t keywords, EventPipeEventLevel provider_level, bool enabled, - EventPipeSessionID session_id) + EventPipeSessionID session_id, + EventPipeProvider *provider) { EventPipeProviderCallbackData *instance = ep_rt_object_alloc (EventPipeProviderCallbackData); ep_raise_error_if_nok (instance != NULL); @@ -238,7 +239,8 @@ ep_provider_callback_data_alloc ( keywords, provider_level, enabled, - session_id) != NULL); + session_id, + provider) != NULL); ep_on_exit: return instance; @@ -298,7 +300,8 @@ ep_provider_callback_data_init ( int64_t keywords, EventPipeEventLevel provider_level, bool enabled, - EventPipeSessionID session_id) + EventPipeSessionID session_id, + EventPipeProvider *provider) { EP_ASSERT (provider_callback_data != NULL); @@ -309,6 +312,7 @@ ep_provider_callback_data_init ( provider_callback_data->provider_level = provider_level; provider_callback_data->enabled = enabled; provider_callback_data->session_id = session_id; + provider_callback_data->provider = provider; return provider_callback_data; } From e6bcef62565971ca8803cf756ae0ef644395bea3 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 15:21:38 -0400 Subject: [PATCH 3/7] [EventPipe] Increment and decrement callback counter --- src/native/eventpipe/ep-provider.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 5fdba63fe1f549..bfe517fd966b1e 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -78,6 +78,11 @@ provider_prepare_callback_data ( EP_ASSERT (provider != NULL); EP_ASSERT (provider_callback_data != NULL); + ep_requires_lock_held (); + + if (provider->callback_func != NULL) + provider->uninvoked_prepared_callbacks_counter++; + return ep_provider_callback_data_init ( provider_callback_data, filter_data, @@ -433,6 +438,13 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data) callback_data /* CallbackContext */); } + EP_LOCK_ENTER (section1) + if (callback_function != NULL) { + EventPipeProvider *provider = provider_callback_data->provider; + provider->uninvoked_prepared_callbacks_counter--; + } + EP_LOCK_EXIT (section1) + ep_on_exit: if (is_event_filter_desc_init) ep_event_filter_desc_fini (&event_filter_desc); From 922baefdfcdde18b88c30343eb1533bc062d70ae Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 15:22:17 -0400 Subject: [PATCH 4/7] [EventPipe] Block event pipe deferred deletion for callbacks --- src/native/eventpipe/ep-provider.c | 5 +++++ src/native/eventpipe/ep.c | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index bfe517fd966b1e..7da4fe7a11a3cf 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -442,6 +442,11 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data) if (callback_function != NULL) { EventPipeProvider *provider = provider_callback_data->provider; provider->uninvoked_prepared_callbacks_counter--; + if (provider->uninvoked_prepared_callbacks_counter == 0 && provider->callback_func == NULL) { + // ep_delete_provider deferred provider deletion and is waiting for all in-flight callbacks + // to complete. This is the last callback, so signal completion. + ep_rt_wait_event_set (&provider->callbacks_complete_event); + } } EP_LOCK_EXIT (section1) diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c index 5b9dcc557266f5..afe796cb6608b4 100644 --- a/src/native/eventpipe/ep.c +++ b/src/native/eventpipe/ep.c @@ -1317,15 +1317,25 @@ ep_delete_provider (EventPipeProvider *provider) // Take the lock to make sure that we don't have a race // between disabling tracing and deleting a provider // where we hold a provider after tracing has been disabled. + bool wait_for_provider_callbacks_completion = false; EP_LOCK_ENTER (section1) if (enabled ()) { // Save the provider until the end of the tracing session. ep_provider_set_delete_deferred (provider, true); + + // the callback func must be previously set to null, + // otherwise callbacks might never stop coming + EP_ASSERT (provider->callback_func == NULL); + if (provider->uninvoked_prepared_callbacks_counter > 0) + wait_for_provider_callbacks_completion = true; } else { config_delete_provider (ep_config_get (), provider); } EP_LOCK_EXIT (section1) + if (wait_for_provider_callbacks_completion) + ep_rt_wait_event_wait (&provider->callbacks_complete_event, EP_INFINITE_WAIT, false); + ep_on_exit: ep_requires_lock_not_held (); return; From 5e02c5e47a5fcb00e5d2c0759d44ac499694e44b Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 16:00:33 -0400 Subject: [PATCH 5/7] [Tests] Reenable eventsourceerror tests --- src/tests/issues.targets | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index a1da9729365589..8ee2b275f03fca 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -404,9 +404,6 @@ https://github.com/dotnet/runtime/issues/68690 - - https://github.com/dotnet/runtime/issues/80666 - Allocates large contiguous array that is not consistently available on 32-bit platforms @@ -420,9 +417,6 @@ https://github.com/dotnet/runtime/issues/66174 - - https://github.com/dotnet/runtime/issues/80666 - From 520635a5090584acd37c22cadddf09141c5da297 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Tue, 6 Aug 2024 16:09:10 -0400 Subject: [PATCH 6/7] Add comment for not taking lock around callback --- src/native/eventpipe/ep-provider.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 7da4fe7a11a3cf..30f777b304fd63 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -374,7 +374,9 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data) { EP_ASSERT (provider_callback_data != NULL); - // Lock should not be held when invoking callback. + // A lock should not be held when invoking the callback, as concurrent callbacks + // may trigger a deadlock with the EventListenersLock as detailed in + // https://github.com/dotnet/runtime/pull/105734 ep_requires_lock_not_held (); const ep_char8_t *filter_data = ep_provider_callback_data_get_filter_data (provider_callback_data); @@ -438,6 +440,7 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data) callback_data /* CallbackContext */); } + // The callback completed, can take the lock again. EP_LOCK_ENTER (section1) if (callback_function != NULL) { EventPipeProvider *provider = provider_callback_data->provider; From d28ed8e44b54e6be1d81441fcd2f3f408d0d15f9 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Wed, 7 Aug 2024 12:19:49 -0400 Subject: [PATCH 7/7] Address feedback Rename counter Add more comments describing the blocking behavior Add comments for potential deadlock scenario --- .../Diagnostics/Tracing/EventPipeEventProvider.cs | 3 +++ src/native/eventpipe/ep-provider.c | 8 ++++---- src/native/eventpipe/ep-provider.h | 7 +++---- src/native/eventpipe/ep.c | 14 +++++++++++--- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs index bef1e1f5d30584..606d149d800db2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs @@ -85,6 +85,9 @@ internal override unsafe void Register(EventSource eventSource) } // Unregister an event provider. + // Calling Unregister within a Callback will result in a deadlock + // as deleting the provider with an active tracing session will block + // until all of the provider's callbacks are completed. internal override void Unregister() { if (_provHandle != 0) diff --git a/src/native/eventpipe/ep-provider.c b/src/native/eventpipe/ep-provider.c index 30f777b304fd63..0c4f4441bba567 100644 --- a/src/native/eventpipe/ep-provider.c +++ b/src/native/eventpipe/ep-provider.c @@ -81,7 +81,7 @@ provider_prepare_callback_data ( ep_requires_lock_held (); if (provider->callback_func != NULL) - provider->uninvoked_prepared_callbacks_counter++; + provider->callbacks_pending++; return ep_provider_callback_data_init ( provider_callback_data, @@ -209,7 +209,7 @@ ep_provider_alloc ( instance->config = config; instance->delete_deferred = false; instance->sessions = 0; - instance->uninvoked_prepared_callbacks_counter = 0; + instance->callbacks_pending = 0; ep_on_exit: return instance; @@ -444,8 +444,8 @@ provider_invoke_callback (EventPipeProviderCallbackData *provider_callback_data) EP_LOCK_ENTER (section1) if (callback_function != NULL) { EventPipeProvider *provider = provider_callback_data->provider; - provider->uninvoked_prepared_callbacks_counter--; - if (provider->uninvoked_prepared_callbacks_counter == 0 && provider->callback_func == NULL) { + provider->callbacks_pending--; + if (provider->callbacks_pending == 0 && provider->callback_func == NULL) { // ep_delete_provider deferred provider deletion and is waiting for all in-flight callbacks // to complete. This is the last callback, so signal completion. ep_rt_wait_event_set (&provider->callbacks_complete_event); diff --git a/src/native/eventpipe/ep-provider.h b/src/native/eventpipe/ep-provider.h index 85cb7bc6cc1a35..4ecedd5d506609 100644 --- a/src/native/eventpipe/ep-provider.h +++ b/src/native/eventpipe/ep-provider.h @@ -42,10 +42,9 @@ struct _EventPipeProvider_Internal { // True if the provider has been deleted, but that deletion // has been deferred until tracing is stopped. bool delete_deferred; - // The number of EventPipeProvider callbacks that have been - // prepared but not yet invoked. - // Used to determine when it is safe to delete a provider. - int64_t uninvoked_prepared_callbacks_counter; + // The number of pending EventPipeProvider callbacks that have + // not completed. + int64_t callbacks_pending; // Event object used to signal eventpipe provider deletion // that all in flight callbacks have completed. ep_rt_wait_event_handle_t callbacks_complete_event; diff --git a/src/native/eventpipe/ep.c b/src/native/eventpipe/ep.c index afe796cb6608b4..ae0af76d64680d 100644 --- a/src/native/eventpipe/ep.c +++ b/src/native/eventpipe/ep.c @@ -1323,16 +1323,24 @@ ep_delete_provider (EventPipeProvider *provider) // Save the provider until the end of the tracing session. ep_provider_set_delete_deferred (provider, true); - // the callback func must be previously set to null, - // otherwise callbacks might never stop coming + // The callback func must be previously set to null, + // otherwise callbacks might never stop coming. EP_ASSERT (provider->callback_func == NULL); - if (provider->uninvoked_prepared_callbacks_counter > 0) + + // Calling ep_delete_provider within a Callback will result in a deadlock + // as deleting the provider with an active tracing session will block + // until all of the provider's callbacks are completed. + if (provider->callbacks_pending > 0) wait_for_provider_callbacks_completion = true; } else { config_delete_provider (ep_config_get (), provider); } EP_LOCK_EXIT (section1) + // Block provider deletion until all pending callbacks are completed. + // Helps prevent the EventPipeEventProvider Unregister logic from + // freeing freeing the provider's weak reference gchandle before + // callbacks using that handle have completed. if (wait_for_provider_callbacks_completion) ep_rt_wait_event_wait (&provider->callbacks_complete_event, EP_INFINITE_WAIT, false);