Respond with 408 status when the server didn't receive the request fully #5680
Conversation
…lly. Motivation: Currently, the server always responds with a 503 status if a `RequestTimeoutException` is raised. However, according to the RFC 9110, the correct response in this case should be a 408 status code. https://httpwg.org/specs/rfc9110.html#status.408 ``` The 408 (Request Timeout) status code indicates that the server did not receive a complete request message within the time that it was prepared to wait. ``` Modification: - Introduced `DecodedHttpRequest.isNormallyClosed()` to check if the request was received fully. - Updated the server to send a 408 response when a request times out and the service didn't receive the request fully. Result: - The server now returns a 408 status if a service didn't receive the request fully and the request times out. - Issue line#5579 has been closed.
| @@ -482,10 +482,8 @@ public CompletableFuture<List<T>> collect(EventExecutor executor, SubscriptionOp | |||
| } | |||
|
|
|||
| private static SubscriptionImpl noopSubscription() { | |||
| final DefaultStreamMessage<?> streamMessage = new DefaultStreamMessage<>(); | |||
There was a problem hiding this comment.
We shouldn't use DefaultStreamMessage because subscription.request is called without subscribing.
core/src/main/java/com/linecorp/armeria/server/DefaultServerErrorHandler.java
Outdated
Show resolved
Hide resolved
| @@ -88,6 +88,8 @@ static DecodedHttpRequest of(boolean endOfStream, EventLoop eventLoop, int id, i | |||
|
|
|||
| void close(Throwable cause); | |||
|
|
|||
| boolean isNormallyClosed(); | |||
There was a problem hiding this comment.
I know what normally means, but it doesn't really hit me. How about?
| boolean isNormallyClosed(); | |
| boolean isClosedSuccessfully(); |
| if (ctx.method() == HttpMethod.GET) { | ||
| // Consider GET requests as having received the request fully. | ||
| status = HttpStatus.SERVICE_UNAVAILABLE; |
There was a problem hiding this comment.
My understanding is that this branch is used for performance optimization. Because, generally, request.isNormallyClosed() should be true for GET method.
There was a problem hiding this comment.
A GET request is allowed to have a request body, thus isNormallyClosed might not be called.
https://stackoverflow.com/questions/978061/http-get-with-request-body
There was a problem hiding this comment.
Is there any reason that we should return 503 even if a request hasn’t fully received for a GET instead of 408?
There was a problem hiding this comment.
I thought we could consider a GET request as having received the request fully.
Do you prefer to return 408 instead?
There was a problem hiding this comment.
I thought returning 408 is more consistent behavior because that does not rely on HTTP method but check if a request is fully received.
There was a problem hiding this comment.
Alright then let me return 408 instead. For the record, a GET request that doesn't have any request body might get 408 as well because this DefaultServerErrorHandler can be called before isNormalClosed() is called.
There was a problem hiding this comment.
I think, in most cases, isNormalClosed() is true when a DecodedHttpRequest is created for a GET request. endOfStream would be true with a GET request having an empty body. As a result, EmptyContentDecodedHttpRequest will be created for which isNormalClosed() is always true.
There was a problem hiding this comment.
That is true. I thought an HTTP/1.1 request was handled differently but it was not after all.
|
Thanks for reviewing. 😉 |
…lly (line#5680) Motivation: Currently, the server always responds with a 503 status if a `RequestTimeoutException` is raised. However, according to the RFC 9110, the correct response in this case should be a 408 status code. https://httpwg.org/specs/rfc9110.html#status.408 ``` The 408 (Request Timeout) status code indicates that the server did not receive a complete request message within the time that it was prepared to wait. ``` Modification: - Introduced `DecodedHttpRequest.isNormallyClosed()` to check if the request was received fully. - Updated the server to send a 408 response when a request times out and the service didn't receive the request fully. Result: - The server now returns a 408 status if a service didn't receive the request fully and the request times out. - Issue line#5579 has been closed.
Motivation:
Currently, the server always responds with a 503 status if a
RequestTimeoutExceptionis raised. However, according to the RFC 9110, the correct response in this case should be a 408 status code. https://httpwg.org/specs/rfc9110.html#status.408Modification:
DecodedHttpRequest.isClosedSuccefully()to check if the request was received fully.Result: