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

fix: handle AggregateExceptions from task continuations #280

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

cprice404
Copy link
Contributor

In Middleware code, if you need to modify the response before it
makes it back out to the caller, you need to use Task.ContinueWith.
However, if an exception is thrown while such a Middleware is
in the stack, the exception will get wrapped in an AggregateException.

Prior to this commit our CacheExceptionMapper would treat this as
an unknown error and we would lose our desired status code mapping.
This commit unwraps AggregateExceptions before the mapping so that
the error codes will be correct.

It also adds debug logs to the exception mapper so that we can
see what's going wrong if other exception types fail to map
properly in the future.

poppoerika
poppoerika previously approved these changes Oct 6, 2022
Copy link
Contributor

@poppoerika poppoerika left a comment

Choose a reason for hiding this comment

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

Looks good to me!

pgautier404
pgautier404 previously approved these changes Oct 6, 2022
Copy link
Contributor

@pgautier404 pgautier404 left a comment

Choose a reason for hiding this comment

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

Nice!

In Middleware code, if you need to modify the response before it
makes it back out to the caller, you need to use `Task.ContinueWith`.
However, if an exception is thrown while such a Middleware is
in the stack, the exception will get wrapped in an AggregateException.

Prior to this commit our CacheExceptionMapper would treat this as
an unknown error and we would lose our desired status code mapping.
This commit unwraps AggregateExceptions before the mapping so that
the error codes will be correct.

It also adds debug logs to the exception mapper so that we can
see what's going wrong if other exception types fail to map
properly in the future.
@cprice404 cprice404 dismissed stale reviews from pgautier404 and poppoerika via 9214bb4 October 6, 2022 23:48
@cprice404 cprice404 force-pushed the handle-continuation-exception-types branch from ca4f6eb to 9214bb4 Compare October 6, 2022 23:48
@cprice404
Copy link
Contributor Author

had to rebase this one

@cprice404 cprice404 merged commit 59d9085 into main Oct 7, 2022
@cprice404 cprice404 deleted the handle-continuation-exception-types branch October 7, 2022 18:50
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

Successfully merging this pull request may close these issues.

3 participants