-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
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>
I have identified the cause of the failure and opened up PR on Jetty (jetty/jetty.project#12579) for further discussions |
ue.isPermanent() | ||
? HttpServletResponse.SC_NOT_FOUND | ||
: HttpServletResponse.SC_SERVICE_UNAVAILABLE, | ||
cause.getMessage()); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious on
- What response code do we give in Jetty 12? Also the RPC Connector mode?
- I see that we are returning 404 but the PR mentions 413
There was a problem hiding this comment.
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>
There was untested case in the This is the reason the additional logic to convert the status code was needed in |
Submitted now via manual 2e3fd5a |
Issue #310
SizeLimitHandlerTest
andSizeLimitIgnoreTest
.SizeLimitHandler
causes 500 response not 413.