Skip to content

fix: remove double logging of BadHttpRequest errors #54395

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
wants to merge 1 commit into from

Conversation

davhdavh
Copy link
Contributor

@davhdavh davhdavh commented Mar 6, 2024

remove double logging of BadHttpRequest errors

Description

SetBadRequestState already logs the error at debug level (where it belongs).
There is no reason to also log it as an application error (at error level), when it isn't an application error, but an IO error.

Fixes #23949

@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Mar 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 6, 2024
@adityamandaleeka
Copy link
Member

This doesn't actually fix #23949 (which is about the error occurring at all, not about the fact that it's logged as an application error).

@davhdavh
Copy link
Contributor Author

davhdavh commented Mar 6, 2024

Depends on your point of view.
It is absolutely correct to throw the exception when a connection error occurs while reading the body.
Now problem is if it an IO exception or Cancelled exception. IMHO, the error happens on IO, so it should be an IO error. And others must have agreed for the problem to stick around for many years.
This PR fixes the fact that the IO exception is logged as an application error, when it is not.
That removes it from normal devs logs, and thus there is no reason to have a bug for it, except perhaps a different bug that goes back to discuss if this should be an IO exception or Cancelled exception.

@amcasey
Copy link
Member

amcasey commented Mar 13, 2024

@davhdavh I find tratcher's argument, that throwing makes logical sense and the caller should do more to suppress the effects, persuasive and I'm not sure I understand the distinction you're drawing between IO exceptions and Cancelled exceptions. Can you please elaborate on why this is the appropriate fix for #23949?

In particular, are all BadHttpRequestExceptions client errors that a server owner wouldn't find interesting? (From the bug, it sounds like it happens under load, suggesting the server is involved.) And what other effects does not setting _applicationError have? It seems like it might impact keep-alive?

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 20, 2024
@amcasey
Copy link
Member

amcasey commented Apr 5, 2024

Closing without prejudice - feel free to create a new one if you have new data or arguments.

@amcasey amcasey closed this Apr 5, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 5, 2024
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 community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException: Unexpected end of request content.
3 participants