Honor ThrowOnEventWriteErrors for exceptions thrown by EventListeners

[MihaZupan]

From #56208 (comment):

EventSource will always propagate exceptions thrown by EventListeners back to the WriteEvent caller, ignoring the ThrowOnEventWriteErrors option.
That can lead to problems if producers of events are not aware of it as @stephentoub described.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

From #56208 (comment):

EventSource will always propagate exceptions thrown by EventListeners back to the WriteEvent caller, ignoring the ThrowOnEventWriteErrors option.
That can lead to problems if producers of events are not aware of it as @stephentoub described.

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Diagnostics.Tracing
Milestone:	6.0.0

[MihaZupan]

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

[stephentoub]

Thanks!

[josalem]

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.

[MihaZupan]

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?

[josalem]

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

[MihaZupan]

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.

[josalem]

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.

[MihaZupan]

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?

[noahfalk]

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.

[josalem]

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.

[noahfalk]

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

[noahfalk]

LGTM

[MihaZupan]

Thanks