Remove AsyncCausalityTracer by logging into TplEventSource directly #38313
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Diagnostics; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace System.Threading.Tasks | ||
| { | ||
| internal static class AsyncCausalityTracer | ||
| { | ||
| internal static void Enable() | ||
| { | ||
| Interlocked.Increment(ref listenerCnt); | ||
| } | ||
|
|
||
| internal static void Disable() | ||
| { | ||
| Interlocked.Decrement(ref listenerCnt); | ||
| } | ||
|
|
||
| internal static bool LoggingOn => listenerCnt > 0; | ||
| private static int listenerCnt = 0; | ||
|
|
||
|
|
||
| // The TraceXXX methods should be called only if LoggingOn property returned true | ||
| // | ||
| [MethodImpl(MethodImplOptions.NoInlining)] // Tracking is slow path. Disable inlining for it. | ||
| internal static void TraceOperationCreation(Task task, string operationName) | ||
| { | ||
| try | ||
| { | ||
| if (LoggingOn) | ||
sywhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| TplEventSource.Log.TraceOperationBegin(task.Id, operationName, RelatedContext: 0); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); | ||
|
There was a problem hiding this comment. What exceptions might get thrown? I'm not clear on why the try/catch is needed here and elsewhere. There was a problem hiding this comment. All these may be a left-over from the original WinRT AsyncCausalityTracer implementation and may not be needed anymore. |
||
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| internal static void TraceOperationCompletion(Task task, AsyncCausalityStatus status) | ||
| { | ||
| try | ||
| { | ||
| if (LoggingOn) | ||
| TplEventSource.Log.TraceOperationEnd(task.Id, status); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); | ||
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| internal static void TraceOperationRelation(Task task, CausalityRelation relation) | ||
| { | ||
| try | ||
| { | ||
| if (LoggingOn) | ||
| TplEventSource.Log.TraceOperationRelation(task.Id, relation); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); | ||
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| internal static void TraceSynchronousWorkStart(Task task, CausalitySynchronousWork work) | ||
| { | ||
| try | ||
| { | ||
| if (LoggingOn) | ||
| TplEventSource.Log.TraceSynchronousWorkBegin(task.Id, work); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); | ||
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| internal static void TraceSynchronousWorkCompletion(CausalitySynchronousWork work) | ||
| { | ||
| try | ||
| { | ||
| if (LoggingOn) | ||
| TplEventSource.Log.TraceSynchronousWorkEnd(work); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogAndDisable(ex); | ||
| } | ||
| } | ||
|
|
||
| // fix for 796185: leaking internal exceptions to customers, | ||
sywhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // we should catch and log exceptions but never propagate them. | ||
| private static void LogAndDisable(Exception ex) | ||
| { | ||
| Disable(); | ||
|
There was a problem hiding this comment. I'm not clear why this is the right thing to do. If an exception occurs (and per my previous comment, I'm not sure why/when that would happen), we decrement the ref count, but we don't actually unregister anything. Doesn't that suggest that the count could just keep decreasing and decreasing? That would seem to a) mean you couldn't re-enable it eventually, and b) in an extreme case, it could wrap around and we'd end up seeing it as enabled again? There was a problem hiding this comment. An exception can be thrown when TplEventSource tries to write events into ETW and an error occurs (i.e. not enough memory). That being said, I'm not entirely sure why we should catch that exception and disable... Like Jan said, it is a remnant from the old WinRT AsyncCausalityTracer. I suppose we can remove this unless @noahfalk has any other thoughts? There was a problem hiding this comment. I think it would be fine to remove these exception catches. I don't see any reason why an exception that was acceptable to throw from TplEventSource would be unacceptable to throw here. |
||
| Debugger.Log(0, "AsyncCausalityTracer", ex.ToString()); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the AsyncCausalityTracer type at all at this point? Could call sites just be updated to use TplEventSource directly, as with all of the other call sites to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need it since the WinRT dependent component from this class is removed - we could just update all the call sites to use TplEventSource directly. Would that be something you'd prefer over adding AsyncCausalityTracer back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be better - especially once the other cruft is deleted and these methods become trivial forwarders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We can look at
TplEventSourceif we want to understand the full list of events that can be emitted. I'd only argue for keepingAsyncCausalityTracerif it were holding state, but that doesn't appear to be the case.