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.

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

Error handler not having access to the original exceptions #1154

Open
xhafan opened this issue Mar 22, 2024 · 7 comments
Open

Error handler not having access to the original exceptions #1154

xhafan opened this issue Mar 22, 2024 · 7 comments

Comments

@xhafan
Copy link

xhafan commented Mar 22, 2024

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 added a commit to xhafan/Rebus that referenced this issue Mar 22, 2024
…ions - DefaultRetryStep using ExceptionInfo factory to create ExceptionInfo instead of newing it up; when configuring Rebus with InMemExceptionInfoFactory [Configure.With(activator).Errors(e => e.UseInMemExceptionInfos())],

an error handler will get an instance of InMemExceptionInfo in HandlePoisonMessage method, with InMemExceptionInfo storing all exceptions within AggregateException; not tested - tests will be added in a subsequent commit when this approach is approved
@xhafan
Copy link
Author

xhafan commented Mar 22, 2024

@mookid8000 I committed an attempt to fix this: xhafan@4be1c8e 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
Copy link
Member

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
Copy link
Author

xhafan commented Mar 23, 2024

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
Copy link
Author

xhafan commented Mar 23, 2024

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

@miljan012
Copy link

miljan012 commented Apr 1, 2024

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
Copy link

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants