Skip to content

Commit

Permalink
Use weak handle for the back reference to EventProvider (#80482)
Browse files Browse the repository at this point in the history 
* Use weak handle for the back reference to EventProvider

This avoids leak when the event provider is orphaned without explicit unregistration.

Fixes #80450

* Delete unnecessary IEventProvider interface

Repurpose no-op event provider as a base class

* Fix warning on wasm
  • Loading branch information
@jkotas
jkotas authored Jan 13, 2023
1 parent dad45a6 commit 32cb6bb
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 82 deletions.
1 change: 0 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,6 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\EventSource.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\EventSourceException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\FrameworkEventSource.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\IEventProvider.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\IncrementingEventCounter.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\IncrementingPollingCounter.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\NativeRuntimeEventSource.cs" />
Expand Down
28 changes: 15 additions & 13 deletions ...libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,29 @@

namespace System.Diagnostics.Tracing
{
internal sealed class EventPipeEventProvider : IEventProvider
internal sealed class EventPipeEventProvider : EventProviderImpl
{
private EventEnableCallback? _enableCallback;
private readonly WeakReference<EventProvider> _eventProvider;
private IntPtr _provHandle;
private GCHandle _gcHandle;

internal EventPipeEventProvider(EventProvider eventProvider)
{
_eventProvider = new WeakReference<EventProvider>(eventProvider);
}

[UnmanagedCallersOnly]
private static unsafe void Callback(byte* sourceId, int isEnabled, byte level,
long matchAnyKeywords, long matchAllKeywords, Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData, void* callbackContext)
{
((EventPipeEventProvider)GCHandle.FromIntPtr((IntPtr)callbackContext).Target!)._enableCallback!(
isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
EventPipeEventProvider _this = (EventPipeEventProvider)GCHandle.FromIntPtr((IntPtr)callbackContext).Target!;
if (_this._eventProvider.TryGetTarget(out EventProvider? target))
target.EnableCallback(isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
}

// Register an event provider.
unsafe void IEventProvider.EventRegister(
EventSource eventSource,
EventEnableCallback enableCallback)
internal override unsafe void Register(EventSource eventSource)
{
_enableCallback = enableCallback;

Debug.Assert(!_gcHandle.IsAllocated);
_gcHandle = GCHandle.Alloc(this);

Expand All @@ -39,7 +41,7 @@ unsafe void IEventProvider.EventRegister(
}

// Unregister an event provider.
void IEventProvider.EventUnregister()
internal override void Unregister()
{
if (_provHandle != 0)
{
Expand All @@ -53,7 +55,7 @@ void IEventProvider.EventUnregister()
}

// Write an event.
unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
internal override unsafe EventProvider.WriteEventErrorCode EventWriteTransfer(
in EventDescriptor eventDescriptor,
IntPtr eventHandle,
Guid* activityId,
Expand Down Expand Up @@ -85,13 +87,13 @@ unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
}

// Get or set the per-thread activity ID.
int IEventProvider.EventActivityIdControl(Interop.Advapi32.ActivityControl controlCode, ref Guid activityId)
internal override int ActivityIdControl(Interop.Advapi32.ActivityControl controlCode, ref Guid activityId)
{
return EventActivityIdControl(controlCode, ref activityId);
}

// Define an EventPipeEvent handle.
unsafe IntPtr IEventProvider.DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion, uint level,
internal override unsafe IntPtr DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion, uint level,
byte *pMetadata, uint metadataLength)
{
return EventPipeInternal.DefineEvent(_provHandle, eventID, keywords, eventVersion, level, pMetadata, metadataLength);
Expand Down
69 changes: 34 additions & 35 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,6 @@ internal enum ControllerCommand
Disable = -3,
}

internal unsafe delegate void EventEnableCallback(
int isEnabled,
byte level,
long matchAnyKeywords,
long matchAllKeywords,
Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData);

/// <summary>
/// Only here because System.Diagnostics.EventProvider needs one more extensibility hook (when it gets a
/// controller callback)
Expand Down Expand Up @@ -72,7 +65,7 @@ internal SessionInfo(int sessionIdBit_, int etwSessionId_)
{ sessionIdBit = sessionIdBit_; etwSessionId = etwSessionId_; }
}

internal IEventProvider m_eventProvider; // The interface that implements the specific logging mechanism functions.
internal EventProviderImpl m_eventProvider; // The implementation of the specific logging mechanism functions.
private byte m_level; // Tracing Level
private long m_anyKeywordMask; // Trace Enable Flags
private long m_allKeywordMask; // Match all keyword
Expand Down Expand Up @@ -113,12 +106,12 @@ internal EventProvider(EventProviderType providerType)
m_eventProvider = providerType switch
{
#if TARGET_WINDOWS
EventProviderType.ETW => new EtwEventProvider(),
EventProviderType.ETW => new EtwEventProvider(this),
#endif
#if FEATURE_PERFTRACING
EventProviderType.EventPipe => new EventPipeEventProvider(),
EventProviderType.EventPipe => new EventPipeEventProvider(this),
#endif
_ => new NoOpEventProvider(),
_ => new EventProviderImpl(),
};
}

Expand All @@ -132,7 +125,7 @@ internal unsafe void Register(EventSource eventSource)
m_providerName = eventSource.Name;
m_providerId = eventSource.Guid;

m_eventProvider.EventRegister(eventSource, new EventEnableCallback(EnableCallBack));
m_eventProvider.Register(eventSource);
}

//
Expand Down Expand Up @@ -185,7 +178,7 @@ protected virtual void Dispose(bool disposing)
//
// We solve by Unregistering after releasing the EventListenerLock.
Debug.Assert(!Monitor.IsEntered(EventListener.EventListenersLock));
m_eventProvider.EventUnregister();
m_eventProvider.Unregister();
}

/// <summary>
Expand All @@ -202,7 +195,7 @@ public virtual void Close()
Dispose(false);
}

private unsafe void EnableCallBack(
internal unsafe void EnableCallback(
int controlCode,
byte setLevel,
long anyKeyword,
Expand Down Expand Up @@ -1129,27 +1122,30 @@ internal unsafe int SetInformation(

#if TARGET_WINDOWS
// A wrapper around the ETW-specific API calls.
internal sealed class EtwEventProvider : IEventProvider
internal sealed class EtwEventProvider : EventProviderImpl
{
private EventEnableCallback? _enableCallback;
private readonly WeakReference<EventProvider> _eventProvider;
private long _registrationHandle;
private GCHandle _gcHandle;

internal EtwEventProvider(EventProvider eventProvider)
{
_eventProvider = new WeakReference<EventProvider>(eventProvider);
}

[UnmanagedCallersOnly]
private static unsafe void Callback(Guid* sourceId, int isEnabled, byte level,
long matchAnyKeywords, long matchAllKeywords, Interop.Advapi32.EVENT_FILTER_DESCRIPTOR* filterData, void* callbackContext)
{
((EtwEventProvider)GCHandle.FromIntPtr((IntPtr)callbackContext).Target!)._enableCallback!(
isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
EtwEventProvider _this = (EtwEventProvider)GCHandle.FromIntPtr((IntPtr)callbackContext).Target!;

if (_this._eventProvider.TryGetTarget(out EventProvider? target))
target.EnableCallback(isEnabled, level, matchAnyKeywords, matchAllKeywords, filterData);
}

// Register an event provider.
unsafe void IEventProvider.EventRegister(
EventSource eventSource,
EventEnableCallback enableCallback)
internal override unsafe void Register(EventSource eventSource)
{
_enableCallback = enableCallback;

Debug.Assert(!_gcHandle.IsAllocated);
_gcHandle = GCHandle.Alloc(this);

Expand All @@ -1170,7 +1166,7 @@ unsafe void IEventProvider.EventRegister(
}

// Unregister an event provider.
void IEventProvider.EventUnregister()
internal override void Unregister()
{
if (_registrationHandle != 0)
{
Expand All @@ -1184,7 +1180,7 @@ void IEventProvider.EventUnregister()
}

// Write an event.
unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
internal override unsafe EventProvider.WriteEventErrorCode EventWriteTransfer(
in EventDescriptor eventDescriptor,
IntPtr eventHandle,
Guid* activityId,
Expand Down Expand Up @@ -1213,15 +1209,15 @@ unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
}

// Get or set the per-thread activity ID.
int IEventProvider.EventActivityIdControl(Interop.Advapi32.ActivityControl ControlCode, ref Guid ActivityId)
internal override int ActivityIdControl(Interop.Advapi32.ActivityControl ControlCode, ref Guid ActivityId)
{
return Interop.Advapi32.EventActivityIdControl(
ControlCode,
ref ActivityId);
}

// Define an EventPipeEvent handle.
unsafe IntPtr IEventProvider.DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion,
internal override unsafe IntPtr DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion,
uint level, byte* pMetadata, uint metadataLength)
{
throw new System.NotSupportedException();
Expand Down Expand Up @@ -1258,19 +1254,19 @@ internal unsafe int SetInformation(
}
#endif

internal sealed class NoOpEventProvider : IEventProvider
#pragma warning disable CA1852 // EventProviderImpl is not derived from in all targets

internal class EventProviderImpl
{
void IEventProvider.EventRegister(
EventSource eventSource,
EventEnableCallback enableCallback)
internal virtual void Register(EventSource eventSource)
{
}

void IEventProvider.EventUnregister()
internal virtual void Unregister()
{
}

unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
internal virtual unsafe EventProvider.WriteEventErrorCode EventWriteTransfer(
in EventDescriptor eventDescriptor,
IntPtr eventHandle,
Guid* activityId,
Expand All @@ -1281,16 +1277,19 @@ unsafe EventProvider.WriteEventErrorCode IEventProvider.EventWriteTransfer(
return EventProvider.WriteEventErrorCode.NoError;
}

int IEventProvider.EventActivityIdControl(Interop.Advapi32.ActivityControl ControlCode, ref Guid ActivityId)
internal virtual int ActivityIdControl(Interop.Advapi32.ActivityControl ControlCode, ref Guid ActivityId)
{
return 0;
}

// Define an EventPipeEvent handle.
unsafe IntPtr IEventProvider.DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion,
internal virtual unsafe IntPtr DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion,
uint level, byte* pMetadata, uint metadataLength)
{
return IntPtr.Zero;
}
}

#pragma warning restore CA1852

}
33 changes: 0 additions & 33 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/IEventProvider.cs
View file

This file was deleted.

0 comments on commit 32cb6bb

Please sign in to comment.