Skip to content

Commit

Permalink
Address feedback
Browse files Browse the repository at this point in the history
Rename counter
Add more comments describing the blocking behavior
Add comments for potential deadlock scenario
  • Loading branch information
mdh1418 committed Aug 7, 2024
1 parent 520635a commit d28ed8e
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions src/native/eventpipe/ep-provider.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 3 additions & 4 deletions src/native/eventpipe/ep-provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 11 additions & 3 deletions src/native/eventpipe/ep.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit d28ed8e

Please sign in to comment.