rebus-org / Rebus

:bus: Simple and lean service bus implementation for .NET
https://mookid.dk/category/rebus
Other
2.26k stars 353 forks source link

Error handler not having access to the original exceptions #1154

Open xhafan opened 3 months ago

xhafan commented 3 months ago

I have this Rebus 7 IErrorHandler implementation to report failed messages on Slack:

public class ReportFailedMessageErrorHandler(
        IErrorHandler errorHandler,
        ISerializer serializer,
        SlackNotificationSender slackNotificationSender,
        IOptions<RuntimeOptions> runtimeOptions,
        LinkGenerator linkGenerator
    )
    : IErrorHandler
{
    public async Task HandlePoisonMessage(
        TransportMessage transportMessage, 
        ITransactionContext transactionContext,
        Exception exception
    )
    {
        await errorHandler.HandlePoisonMessage(transportMessage, transactionContext, exception);

        var rebusMessage = await serializer.Deserialize(transportMessage);

        var message = rebusMessage.Body as BaseMessage;
        Guard.Hope(message != null, nameof(message) + " is null");

        var shouldNotifyOnFail = message.GetType().GetCustomAttribute<NotifyOnFailAttribute>() != null;
        if (shouldNotifyOnFail && exception.InnerException is not IgnoreNotificationException)
        {
            var jobGuid = message switch
            {
                Command command => command.Guid,
                Event => Guid.Parse(rebusMessage.Headers[Headers.MessageId]),
                _ => throw new NotSupportedException($"Unsupported BaseMessage type {message.GetType().FullName}")
            };
            var failedMessageTypeName = message.GetType().Name;
            await slackNotificationSender.SendNotification(
                $"{failedMessageTypeName} failed",
                $"{_GetSlackJobLink(jobGuid, failedMessageTypeName)} failed on {runtimeOptions.Value.Environment}" +
                $"{(exception.InnerException is UserException ? $": {exception.InnerException.Message}" : ".")}",
                SlackNotificationSenderIconEmojiConstants.Warning,
                exception.InnerException is UserException
                    ? SlackNotificationSenderChannel.ContentChannel
                    : SlackNotificationSenderChannel.DevelopmentChannel
            );
        }
    }

    private string _GetSlackJobLink(Guid jobGuid, string messageTypeName)
    {
        return $"<{runtimeOptions.Value.Url.TrimEnd('/')}{_GetJobUrl(jobGuid)}|{messageTypeName}>";
    }

    private string _GetJobUrl(Guid jobGuid)
    {
        ...
    }
}

It uses Exception.InnerException to detect the exception thrown in the message handler, and behave differently based on the exception type.

In Rebus 8, Exception has been changed to ExceptionInfo without access to InnerException. I refactored the code to this:

    public async Task HandlePoisonMessage(
        TransportMessage transportMessage, 
        ITransactionContext transactionContext,
        ExceptionInfo exception
    )
    {
        await errorHandler.HandlePoisonMessage(transportMessage, transactionContext, exception);

        var rebusMessage = await serializer.Deserialize(transportMessage);

        var message = rebusMessage.Body as BaseMessage;
        Guard.Hope(message != null, nameof(message) + " is null");

        var shouldNotifyOnFail = message.GetType().GetCustomAttribute<NotifyOnFailAttribute>() != null;
        if (shouldNotifyOnFail && !exception.Details.Contains(nameof(IgnoreNotificationException)))
        {
            var jobGuid = message switch
            {
                Command command => command.Guid,
                Event => Guid.Parse(rebusMessage.Headers[Headers.MessageId]),
                _ => throw new NotSupportedException($"Unsupported BaseMessage type {message.GetType().FullName}")
            };
            var failedMessageTypeName = message.GetType().Name;
            await slackNotificationSender.SendNotification(
                $"{failedMessageTypeName} failed",
                $"{_GetSlackJobLink(jobGuid, failedMessageTypeName)} failed on {runtimeOptions.Value.Environment}" +
                $"{(exception.Details.Contains(nameof(UserException)) ? $": {exception.Message}" : ".")}",
                SlackNotificationSenderIconEmojiConstants.Warning,
                exception.Details.Contains(nameof(UserException))
                    ? SlackNotificationSenderChannel.ContentChannel
                    : SlackNotificationSenderChannel.DevelopmentChannel
            );
        }
    }

Not ideal in my opinion that the InnerException disappeared, and it's now doing a stringology (exception.Details.Contains(nameof(UserException))). Is there a better way in Rebus how to implement failed message reporting, and get access to the thrown inner exception message?

Also reported on SO: https://stackoverflow.com/questions/77804512/rebus-7-ierrorhandler-implementation-not-compatible-with-rebus-8

xhafan commented 3 months ago

@mookid8000 I committed an attempt to fix this: https://github.com/xhafan/Rebus/commit/4be1c8e0a215a3f903566e31da30503e29517a88 Could you please have a look and let me know if you would approve this approach. If yes, I will add unit tests, and create a pull request. If no, please suggest improvements 🙂. Thank you.

mookid8000 commented 3 months ago

Hi @xhafan , did you read the section about exception info on the wiki? 👉 https://github.com/rebus-org/Rebus/wiki/Automatic-retries-and-error-handling#exception-information

xhafan commented 3 months ago

Yes, I have read it. But it does not work for me as DefaultRetryStep news up ExceptionInfo directly instead of using ExceptionInfo factory, and as a result the error handler gets an instance of ExceptionInfo and not InMemExceptionInfo.

xhafan commented 3 months ago

@mookid8000 Is my implementation of IErrorHandler correct, or should I try to use IFailed<TMessage> instead?

miljan012 commented 2 months ago

I can report the same issue.

It seems that inside of the method HandlePoisonMessage in a custom IErrorHandler, we cannot cast the ExceptionInfo exception parameter to the InMemExceptionInfo or any other custom ExceptionInfo.

Here's the exception when using the default InMemExceptionInfo:

image

Here's the InMemExceptionInfo configuration: image

I've read the wiki and tried out a custom IExceptionInfoFactory, as well, but with the same result.

Any suggestion, workaround or a fix, please? We rely on a custom exceptions with specific properties, so accessing only the exception type or message from the base ExceptionInfo is not enough.

Thanks!

edward991 commented 2 months ago

We have the same issue described in the previous comments. The original exception is being lost, and we cannot access it in the HandlePoisonMessage of IErrorHandler, as the exception info cannot be cast to InMemExceptionInfo. Any fix so far?

JeremySkinner commented 1 month ago

We've run into the same issue with the original exception being lost by the time it reaches a custom IErrorHandler. This seems to happen because of the GetAggregateException method in DefaultRetryStep, which creates a new ExceptionInfo and simply concatenates the messages of the child ExceptionInfos, again losing the context of the original instance.

GetAggregateException is unfortunately static non-virtual so can't be overridden without re-implementing the entire retry step.

My suggestion would be to update the DefaultRetryStep.GetAggregateException method to return a custom ExceptionInfo subclass which internally holds a reference to the child ExceptionInfos (from which the original exception could then be retrieved if required. Ideally if this method could be made protected virtual rather than private static this would be extremely useful for customising the process too.