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

Make status code configurable #723

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Make status code configurable #723

wants to merge 1 commit into from

Conversation

adwsingh
Copy link

@adwsingh adwsingh commented Apr 21, 2024

@markt-asf
Copy link
Contributor

This PR doesn't appear to change the current behaviour.

@adwsingh
Copy link
Author

adwsingh commented Aug 13, 2024

@markt-asf The intention of the PR was to demonstrate and get feedback on if we could safely not close a connection on a application sent 4xx but always safely close it on HTTP parsing failures. I wanted to get feedback on the approach before I sink more time into this since there were concerns in the mailing list that attempting to do such a thing was not safe at all.

@markt-asf
Copy link
Contributor

Ah, understood. My view is that this is a neat trick but it isn't sufficient. There are a bunch of places where Tomcat calls response.sendError(400,"reason") where we also want the connection to be closed. e.g. CoyoteAdapter. I don't see how we can differentiate between those calls and calls from the application that do not need to be fatal.
I keep coming back to the view that applications should not be using 400 responses for this. Those applications that need to use HTTP response codes should define their own 4xx status code(s).

@adwsingh
Copy link
Author

adwsingh commented Aug 14, 2024

@markt-asf I think Tomcat reserving existing HTTP 4xx responses solely for its internal use is very restrictive to the application.
What do you feel about adding a new field in the CoyoteResponse called shouldCloseConnection that defaults to true and we set it to false once we are sure we are entering application layer. On how we detect we are entering the application layer is something I would need to deep dive on.

@markt-asf
Copy link
Contributor

Tomcat isn't reserving all 4xx responses. It will close the connection if a small sub-set of those status codes is used and does so to avoid various potential security issues. Applications still have over 70 4xx codes to use that have not been defined by any RFC.

The use of async has made determining if an error is set by the application or the container quite a bit trickier.

I remain to be convinced both that applications should be using the 400 status code and that differentiating between an application set 400 status code and container set 400 status code is possible without excessive complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants