Skip to content

Don't cache Endpoints if a source throws #43729

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

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Sep 3, 2022

Description

Don't cache Endpoints in CompositeEndpointDataSource if a child EndpointDataSource source throws. CompositeEndpointDataSource is almost always the root EndpointDataSource, so this affects most apps.

Fixes #43693

Customer Impact

Without this fix, you do not see endpoint generation errors after the first request. You should see something like:

Developer exception page

But instead get 404s after the first failed request:

404 HTTP status page

We should be consistently throwing exceptions to outer middleware like the one responsible for the developer exception page and producing 500s instead of 404s.

Regression?

  • Yes
  • No

This is a regression introduced in 7.0-preview6 by #42195.

Risk

  • High
  • Medium
  • Low

This is merely waiting to cache a list later after we know nothing threw while generating the list's elements.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@Pilchie

@halter73 halter73 added Servicing-consider Shiproom approval is required for the issue feature-routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Sep 3, 2022
@halter73 halter73 added this to the 7.0-rc2 milestone Sep 3, 2022
@halter73 halter73 requested a review from javiercn as a code owner September 3, 2022 03:04
@ghost ghost added the area-runtime label Sep 3, 2022
@ghost
Copy link

ghost commented Sep 3, 2022

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@halter73 halter73 force-pushed the halter73/43693 branch 2 times, most recently from eee92e2 to d0f247d Compare September 3, 2022 03:20
@Pilchie Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 6, 2022
@Pilchie
Copy link
Member

Pilchie commented Sep 6, 2022

Approved for .NET 7 RC2.

@BrennanConroy BrennanConroy merged commit c36a0e0 into release/7.0 Sep 6, 2022
@BrennanConroy BrennanConroy deleted the halter73/43693 branch September 6, 2022 21:12
@halter73 halter73 linked an issue Sep 6, 2022 that may be closed by this pull request
1 task
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Endpoint routing failures are cached
6 participants