Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Honor ThrowOnEventWriteErrors for exceptions thrown by EventListeners #56232

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Honor ThrowOnEventWriteErrors for exceptions thrown by EventListeners #56232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ...raries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestsWriteEventToListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,5 +448,49 @@ public void Test_EventSourceCreatedEvents_AfterListener()

TestUtilities.CheckNoEventSourcesRunning("Stop");
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void Test_EventListenerThrows_ExceptionIsNotRethrownToCaller(bool setThrowOnEventWriteErrorsFlag)
{
TestUtilities.CheckNoEventSourcesRunning("Start");

using (var log = new EventSourceTest(throwOnEventWriteErrors: setThrowOnEventWriteErrorsFlag))
{
using (var listener = new EventListenerListener())
{
listener.EventSourceSynchronousEnable(log);

var thrownException = new Exception("Oops");
string outOfBandMessage = null;

listener.EventWritten += (_, e) =>
{
if (e.EventId == 0)
{
outOfBandMessage = e.Message;
}

throw thrownException;
};

try
{
log.Event0();
Assert.False(setThrowOnEventWriteErrorsFlag);
}
catch (EventSourceException ex)
{
Assert.True(setThrowOnEventWriteErrorsFlag);
Assert.Same(thrownException, ex.InnerException);
}

Assert.Contains(thrownException.Message, outOfBandMessage);
}
}

TestUtilities.CheckNoEventSourcesRunning("Stop");
}
}
}
5 changes: 3 additions & 2 deletions src/libraries/System.Diagnostics.Tracing/tests/CustomEventSources/EventSourceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ namespace SdtEventSources
[EventSource(Guid = "69e2aa3e-083b-5014-cad4-3e511a0b94cf", Name = "EventSourceTest")]
public sealed class EventSourceTest : EventSource
{
public EventSourceTest(bool useSelfDescribingEvents = false)
: base(true)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you may have been missing on some coverage here since useSelfDescribingEvents was being ignored

public EventSourceTest(bool useSelfDescribingEvents = false, bool throwOnEventWriteErrors = false)
: base((useSelfDescribingEvents ? EventSourceSettings.EtwSelfDescribingEventFormat : EventSourceSettings.EtwManifestEventFormat)
| (throwOnEventWriteErrors ? EventSourceSettings.ThrowOnEventWriteErrors : 0))
{ }

protected override void OnEventCommand(EventCommandEventArgs command)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2121,7 +2121,7 @@ internal unsafe void DispatchToAllListeners(EventWrittenEventArgs eventCallbackA
}
}

if (lastThrownException != null)
if (lastThrownException != null && ThrowOnEventWriteErrors)
{
throw new EventSourceException(lastThrownException);
}
Comment on lines -2124 to 2127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason that this is bubbling up in EventSource is not actually because we aren't checking ThrowOnEventWriteErrors, but rather that EventListener is rethrowing any exception as an EventSourceException which gets special cased in the rethrow logic in EventSource:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

Lines 1382 to 1388 in 0c20095

catch (Exception ex)
{
if (ex is EventSourceException)
throw;
else
ThrowEventSourceException(m_eventData[eventId].Name, ex);
}

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

Lines 2341 to 2393 in 0c20095

private void ThrowEventSourceException(string? eventName, Exception? innerEx = null)
{
// If we fail during out of band logging we may end up trying
// to throw another EventSourceException, thus hitting a StackOverflowException.
// Avoid StackOverflow by making sure we do not recursively call this method.
if (m_EventSourceExceptionRecurenceCount > 0)
return;
try
{
m_EventSourceExceptionRecurenceCount++;
string errorPrefix = "EventSourceException";
if (eventName != null)
{
errorPrefix += " while processing event \"" + eventName + "\"";
}
// TODO Create variations of EventSourceException that indicate more information using the error code.
switch (EventProvider.GetLastWriteEventError())
{
case EventProvider.WriteEventErrorCode.EventTooBig:
ReportOutOfBandMessage(errorPrefix + ": " + SR.EventSource_EventTooBig);
if (ThrowOnEventWriteErrors) throw new EventSourceException(SR.EventSource_EventTooBig, innerEx);
break;
case EventProvider.WriteEventErrorCode.NoFreeBuffers:
ReportOutOfBandMessage(errorPrefix + ": " + SR.EventSource_NoFreeBuffers);
if (ThrowOnEventWriteErrors) throw new EventSourceException(SR.EventSource_NoFreeBuffers, innerEx);
break;
case EventProvider.WriteEventErrorCode.NullInput:
ReportOutOfBandMessage(errorPrefix + ": " + SR.EventSource_NullInput);
if (ThrowOnEventWriteErrors) throw new EventSourceException(SR.EventSource_NullInput, innerEx);
break;
case EventProvider.WriteEventErrorCode.TooManyArgs:
ReportOutOfBandMessage(errorPrefix + ": " + SR.EventSource_TooManyArgs);
if (ThrowOnEventWriteErrors) throw new EventSourceException(SR.EventSource_TooManyArgs, innerEx);
break;
default:
if (innerEx != null)
{
innerEx = innerEx.GetBaseException();
ReportOutOfBandMessage(errorPrefix + ": " + innerEx.GetType() + ":" + innerEx.Message);
}
else
ReportOutOfBandMessage(errorPrefix);
if (ThrowOnEventWriteErrors) throw new EventSourceException(innerEx);
break;
}
}
finally
{
m_EventSourceExceptionRecurenceCount--;
}
}

All EventSourceExceptions get treated as errors caused by EventSource and always get rethrown, while other errors get encoded as out of band messages and rethrown based on the config value.

I think the correct path here is to simply rethrow the exception that occurred in EventListener and allow EventSource to handle it according to the config. We may want to wrap all thrown exceptions during the EventListener dispatch phase into an AggregateException rather than only throwing the last one. We can save on allocations and whatnot by skipping that logic based on the config value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simply rethrow the exception that occurred in EventListener and allow EventSource to handle it according to the config

That is what the PR effectively does. It only throws the exception according to the filter, in which case it gets special-cased and propagates through to the caller. The OutOfBand message is also logged explicitly.

How do you recommend it should be changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an exception thrown in a user's OnEventWritten should be special cased as an EventSourceException.

I think we should do something like this (pseudo-code ish):

List<Exception> exceptions = null;
if (ThrowOnEventWriteErrors)
	exceptions = new List<Exception>();

foreach (var listener in listeners)
{
	try
	{
		listener.OnEventWritten(...);
	}
	catch (Exception ex)
	{
		if (ex is EventSourceException) // we only special case EventSource errors and not user errors
			throw;
		else
			exceptions?.Add(ex);
	}
}

if (exceptions?.Count() ?? 0 > 0)
	throw new AggregateException(exceptions); // This should get picked up in ThrowEventSourceException in EventSource

Copy link
Member Author

@MihaZupan MihaZupan Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnEventWritten is just invoking the user code, it should never throw EventSourceException.

ThrowEventSourceException seems to only be handling P/Invoke errors. From what I can tell, using it for arbitrary exceptions could mix user errors with old EventProvider errors.

I can of course change it to throw an AggregateException, but that's just a detail after we decide on how.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should never throw EventSourceException.

It shouldn't. The code, as written, is wrapping any exception thrown in OnEventWritten as an EventSourceException as though it were an exception thrown by EventSource itself. This will be caught by the try/catch in WriteEventVarargs and then rethrown regardless of the value of ThrowOnEventWriteErrors because EventSourceException is special cased. This is what caused the bad behavior in #56208.

All other exceptions thrown during a call to EventWriteVarargs get fed through ThrowEventSourceException where it will send an out of band message and then optionally rethrow the exception based on the value of ThrowOnEventWriteErrors.

We shouldn't wrap an exception thrown from OnEventWritten in an EventSourceExcpetion since it's an exception from user code, not EventSource.

ThrowEventSourceException seems to only be handling P/Invoke errors. From what I can tell, using it for arbitrary exceptions could mix user errors with old EventProvider errors.

All exceptions thrown on the writing path should get passed through this method since it's where the value of ThrowOnEventWriteErrors is checked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThrowOnEventWriteErrors is looking at the EventProvider errors:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

Line 2359 in 0c20095

switch (EventProvider.GetLastWriteEventError())

If you give it exceptions thrown by user code in EventListener, it may report completely unrelated reasons for the error.

We shouldn't wrap an exception thrown from OnEventWritten in an EventSourceExcpetion since it's an exception from user code, not EventSource.

But passing it to ThrowEventSourceException will wrap it in EventSourceException anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an exception thrown in a user's OnEventWritten should be special cased as an EventSourceException

Not doing the wrapping at all would be a further breaking change, but I assume what you meant was not to do it inside this function and instead defer the wrapping to occur one layer higher up?
All else being equal it would be nice to have fewer places doing that wrapping, but the weird coupling ThrowEventSourceException has with native p/invoke return codes does suggest it isn't ideal for a general purpose wrapping mechanism either. If I was writing all this code from scratch I'd probably factor it differently, but given what is already present I think @MihaZupan's edit is a good spot to be and a more substantial refactor is more churn than it is worth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I assume what you meant was not to do it inside this function and instead defer the wrapping to occur one layer higher up?

Yes, that's what I meant. We already do the wrapping in ThrowEventSourceException which is a centralized spot for handling EventSource exceptions that happen during write. My hope was to not check this config too many other places since we already have a try/catch around all the calls to WriteEvent* that direct errors through this code.

At this point, I'm inclined to not push the point further and just go with the patch as-written. I may try to come back a little later and clean things up a bit, though, since I spotted a few unfortunate things while reading through this code path. Mainly, ThrowEventSourceException checks the last set provider error code. The value is a static int shared across all Providers, so it isn't threadsafe to assume the exception being thrown at that point in time is the one that set the return code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josalem - I think you need to change your review status, it is still set on request changes

Expand Down