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

asp .net 5 can't return json with 404 status code #32046

Closed
VladislavMorozov opened this issue Apr 21, 2021 · 15 comments · Fixed by #32198
Closed

asp .net 5 can't return json with 404 status code #32046

VladislavMorozov opened this issue Apr 21, 2021 · 15 comments · Fixed by #32198
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug.
Milestone

Comments

@VladislavMorozov
Copy link

I use .net 5.0.202 with system.text,json.
I have exception handler
image
With processing errors.
image

(failed)net::ERR_INCOMPLETE_CHUNKED_ENCODING

image

@VladislavMorozov
Copy link
Author

Other http codes (4xx) return a valid json

@VladislavMorozov
Copy link
Author

test project
WebApplication1.zip

@mairaw mairaw transferred this issue from dotnet/core Apr 21, 2021
@fvoronin
Copy link
Contributor

I did a little investigation and found that the problem occurs when we set the StatusCode = 404 in the response. In this case, the web server transmitting the response in "Transfer-Encoding: chunked" mode does not transmit a zero-length chunk at the end of the response body.

Compare. As now:

HTTP/1.1 404 Not Found
Date: Thu, 22 Apr 2021 06:40:36 GMT
Content-Type: application/problem+json
Server: Kestrel
Cache-Control: no-cache,no-store
Pragma: no-cache
Transfer-Encoding: chunked
Expires: -1

75
{"title":"title","status":404,"detail":"details","traceId":"00-93249638518c924dafd48997d28c8303-ccd2508c5de5a34d-00"}

As it should be:

HTTP/1.1 500 Internal Server Error
Date: Thu, 22 Apr 2021 06:40:03 GMT
Content-Type: application/problem+json
Server: Kestrel
Cache-Control: no-cache,no-store
Pragma: no-cache
Transfer-Encoding: chunked
Expires: -1

75
{"title":"title","status":404,"detail":"details","traceId":"00-0bad9468621ebd4981854389e389ba9d-29a0cf6cc2c0e44d-00"}
0


@Tratcher, I'd like to take a deeper look at it.

@davidfowl
Copy link
Member

@VladislavMorozov
Copy link
Author

But before it works on .net 5 :) Thanks for help

@VladislavMorozov
Copy link
Author

@davidfowl It works with https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.exceptionhandleroptions.allowstatuscode404response?view=aspnetcore-5.0
Thanks. But I don't understand how this works on previous versions of .net 5 :)

@davidfowl
Copy link
Member

But I don't understand how this works on previous versions of .net 5 :)

Because we changed the behavior. https://docs.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/5.0/middleware-exception-handler-throws-original-exception

We added this API later but never updated the original change it seems:

#26567

cc @JunTaoLuo @Tratcher

@VladislavMorozov
Copy link
Author

@davidfowl It's okay that server return (failed)net::ERR_INCOMPLETE_CHUNKED_ENCODING ?

@Tratcher
Copy link
Member

@davidfowl It's okay that server return (failed)net::ERR_INCOMPLETE_CHUNKED_ENCODING ?

That means the rethrown exception caused the connection to be closed and the response aborted. There's another issue where we disabled the 404 rethrow if there was a body to prevent this kind of abort.

@Tratcher
Copy link
Member

Correction, @JunTaoLuo and I talked about adding a HasStarted check here, but it doesn't look like it's happened yet.

if (context.Response.StatusCode != StatusCodes.Status404NotFound || _options.AllowStatusCode404Response)

@JunTaoLuo do you remember if there's another issue for that, or did we change our minds?

@Tratcher Tratcher reopened this Apr 22, 2021
@davidfowl davidfowl added the bug This issue describes a behavior which is not expected - a bug. label Apr 23, 2021
@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Apr 23, 2021
@ghost
Copy link

ghost commented Apr 23, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@BrennanConroy
Copy link
Member

Triage: we think there is another issue for this. TLDR; we should add the HasStarted check on the body at the end of the error handler.

@davidfowl
Copy link
Member

Mark as up for grabs?

@JunTaoLuo
Copy link
Contributor

Ah, the HasStarted check was proposed in #31024 (comment) but I neglected to do that since during triage we decided to add more useful exception message. I can add the check since it should be a one line change.

@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants