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

Fixes for SizeLimitHandlerTest #313

Closed
wants to merge 6 commits into from
Closed

Conversation

lachlan-roberts
Copy link
Collaborator

Issue #310

  • reduce duplication between SizeLimitHandlerTest and SizeLimitIgnoreTest.
  • add parameterization for jetty 9.4 with HTTP mode.
  • fix issue in HTTP mode where SizeLimitHandler causes 500 response not 413.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts
Copy link
Collaborator Author

I have identified the cause of the failure and opened up PR on Jetty (jetty/jetty.project#12579) for further discussions

Comment on lines 150 to 154
ue.isPermanent()
? HttpServletResponse.SC_NOT_FOUND
: HttpServletResponse.SC_SERVICE_UNAVAILABLE,
cause.getMessage());
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious on

  1. What response code do we give in Jetty 12? Also the RPC Connector mode?
  2. I see that we are returning 404 but the PR mentions 413

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The UnavailableException has nothing to do with the SizeLimitHandlerTest, only the BadMessageException does. Previously in this method we were just sending 500 response, but the response really depends on the exception, so in case of a BadMessageException we should send a 400 and with a UnavailableException we send either 404 or 503 depending if its permanently unavailable.

The Jetty 12 runtime does the same thing just through a different mechanism, it will not even reach this point, it handles it internal to the ServletChannel and writes the error itself with the correct code. In the case that it did reach this point in the Jetty 12 runtime (by failing the callback), then it uses Response.writeError which will deal with HttpException to send a correct response code.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@lachlan-roberts
Copy link
Collaborator Author

There was untested case in the SizeLimitHandlerTest for jetty94 with appengine.use.HttpConnector==true, and testRequestContentAboveMaxLengthGzip was failing because the client was receiving 500 response instead of a 413 response.

This is the reason the additional logic to convert the status code was needed in JettyHttpHandler.

@ludoch
Copy link
Collaborator

ludoch commented Dec 3, 2024

Submitted now via manual 2e3fd5a

@ludoch ludoch closed this Dec 3, 2024
@lachlan-roberts lachlan-roberts deleted the sizeLimitHandlerTest branch December 4, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants