Skip to content

StatusCodePagesMiddleware: Improving response started verification #46576

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

Closed

Conversation

brunolins16
Copy link
Member

Fix #45678

@brunolins16 brunolins16 requested a review from Tratcher February 10, 2023 23:47
@ghost ghost added the area-runtime label Feb 10, 2023
@brunolins16 brunolins16 added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Feb 11, 2023
@@ -26,21 +26,26 @@ public StatusCodePagesOptions()

if (context.HttpContext.RequestServices.GetService<IProblemDetailsService>() is { } problemDetailsService)
{
await problemDetailsService.WriteAsync(new ()
await problemDetailsService.WriteAsync(new()
{
HttpContext = context.HttpContext,
ProblemDetails = { Status = statusCode }
});
}

Copy link
Member

@Tratcher Tratcher Feb 11, 2023

Choose a reason for hiding this comment

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

Checking the content-length and content-type is weird, just make the original code an else block.

Suggested change
else
{
context.HttpContext.Response.ContentType = "text/plain";
await context.HttpContext.Response.WriteAsync(body);
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just saw your comment on the issue... I'll revisit this next week, but I still don't think we want to rely on checking content-length or content-type to determine the presence of a buffered body.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! I don't like it as well but since we were using it already, I decided to suggest it. I tried a repro to see if I can find something reliable but as I mentioned, in the issue, I was not able to repro.

@brunolins16
Copy link
Member Author

Closing this PR to work on a more appropriated fix.

@ghost
Copy link

ghost commented Feb 13, 2023

Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
2 participants