From d28ed8e44b54e6be1d81441fcd2f3f408d0d15f9 Mon Sep 17 00:00:00 2001 From: mdh1418 Date: Wed, 7 Aug 2024 12:19:49 -0400 Subject: [PATCH] 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);