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

Add an option to allow 404 responses from ExceptionHandlers #26567

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Oct 2, 2020

Addresses #26528

Description

We addressed the issue #21510 in RC1 but introduced a breaking change in the default behaviour of the middleware. Specifically, we decided that a 404 response from the exception handler should be considered an error in the server and we will rethrow the original exception instead of returning the 404 response. This is to address a common pitfall where the exception handler middleware was misconfigured with a non-existent path.

However, the change in default behaviour regressed use cases where the exception handler intentionally produces a 404 response as mentioned in #26528. This PR introduces a fix where we add an option on the middleware to allow such responses. When enabled, this option considers 404 responses produced by the exception handler to be valid and will not rethrow the original exception.

Customer Impact

This issue was reported by @danikun in #26528. The breaking change introduced in 5.0 RC1 regressed the customer's exception handling logic which maps some exceptions to 404s. There is no acceptable workaround. We considered alternatives such as invoking CompleteAsync in the exception handler to flush the 404 response but this won't work in all environments; for example, unit testing using TestServer would not work since it doesn't implement CompleteAsync as discussed in #25288 (comment).

Regression?

Yes this is a regression that was introduced in #25062 in 5.0 RC1

Risk

Low. The solution is limited to this middleware and an unit test is added to verify its correctness.

@JunTaoLuo JunTaoLuo added the Servicing-consider Shiproom approval is required for the issue label Oct 2, 2020
@JunTaoLuo JunTaoLuo added this to the 5.0.0 milestone Oct 2, 2020
@ghost ghost added the area-middleware label Oct 2, 2020
@ghost
Copy link

ghost commented Oct 2, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@Tratcher
Copy link
Member

Tratcher commented Oct 2, 2020

@danikun

@JunTaoLuo
Copy link
Contributor Author

Given this will require 5.0 shiproom approval, it's unlikely this will be merged today. If I don't respond promptly while I'm OOF, I nominate @Tratcher to merge as needed.

@danikun
Copy link

danikun commented Oct 5, 2020

Thanks! Good job

@JunTaoLuo
Copy link
Contributor Author

@Pilchie Is this being looked at by shiproom?

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 6, 2020
@Pilchie
Copy link
Member

Pilchie commented Oct 6, 2020

@JunTaoLuo look again 😉

@JunTaoLuo JunTaoLuo merged commit 8555002 into release/5.0 Oct 6, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/exception-handler-option branch October 6, 2020 17:24
@davidfowl
Copy link
Member

davidfowl commented Apr 22, 2021

This isn't documented as part of that original breaking change.

@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants