Fix not to use `Http{Response,Status}Exception` internally #4117

ikhoon · 2022-02-25T12:56:17Z

Motivation:

An HttpResponseException has an HttpResponse. The thrown response
is extracted by the default ServerErrorHandler and converted to
a normal HttpResponse. There are downsides to HttpResponseException.
As the response is wrapped by HttpResponseException,
decorators can not access the HttpObjects of the response in the
exception using mapXXX.

Users may find it hard to mutate the thrown HttpResponse because
recovery should be done first. Therefore, it would be better not to use
HttpResponseException internally or recover it before passing
the response to decorators so that users use HttpResponse.mapXXX to
transform a returned response in decorators.

Modifications:

Add HttpFile.ofRedirect(location) to indicate a file in a different
location.
Recover an Http{Response,Status}Exception thrown by
HealthCheckUpdateHandler
Update Javadoc in detail for Http{Response,Status}Exception
Correctly propagate the cause of Http{Response,Status}Exception in
THttpService

Result:

You can now mutate a redirect response from FileService using
mapHeaders
You no longer see an HttpResponseException or
an HttpStatusException from built-in services.
Fixes HttpResponse.mapHeaders doesn't work with FileService redirect #4056

Motivation: An `HttpResponseException` has an `HttpResponse`. So the default `ServerErrorHandler` extracts the `HttpResponse` from the exception. As the response is wrapped by `HttpResponseException`, decorators can not access the `HttpObject`s of the response in the exception. Users may find it hard to mutate the thrown `HttpResponse` because recovery should be done first. So it would be better not to use `HttpResponseException` or recover it before passing a response to decorators. Modifications: - Add `HttpFile.ofRedirect(location)` to indicate a file in a different location. - Recover an `Http{Response,Status}Exception thrown by `HealthCheckUpdateHandler` - Update Javadoc in detail for `Http{Response,Status}Exception` - Correctly propagate the cause of `Http{Response,Status}Exception` in `THttpService` Result: - You can now mutate a redirect response from `FileService` using `mapHeaders` - You no longer see an `HttpResponseException` or an `HttpStatusException` from built-in services. - Fixes line#4056

codecov · 2022-02-28T07:07:17Z

Codecov Report

Merging #4117 (353c02d) into master (829992a) will increase coverage by 0.05%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master    #4117      +/-   ##
============================================
+ Coverage     73.18%   73.24%   +0.05%     
- Complexity    16126    16137      +11     
============================================
  Files          1405     1405              
  Lines         61669    61694      +25     
  Branches       7765     7767       +2     
============================================
+ Hits          45134    45185      +51     
+ Misses        12612    12584      -28     
- Partials       3923     3925       +2

Impacted Files	Coverage Δ
...linecorp/armeria/server/HttpResponseException.java	`78.57% <ø> (ø)`
...m/linecorp/armeria/server/HttpStatusException.java	`75.00% <ø> (ø)`
.../com/linecorp/armeria/server/file/FileService.java	`82.56% <ø> (-0.15%)`	⬇️
...r/healthcheck/DefaultHealthCheckUpdateHandler.java	`44.18% <0.00%> (-1.06%)`	⬇️
...ava/com/linecorp/armeria/server/file/HttpFile.java	`85.29% <100.00%> (+0.91%)`	⬆️
...ecorp/armeria/server/file/NonExistentHttpFile.java	`83.33% <100.00%> (+6.41%)`	⬆️
...m/linecorp/armeria/server/thrift/THttpService.java	`71.96% <100.00%> (-0.35%)`	⬇️
...rnal/common/GracefulConnectionShutdownHandler.java	`68.08% <0.00%> (-6.39%)`	⬇️
...inecorp/armeria/server/file/StreamingHttpFile.java	`54.78% <0.00%> (-2.61%)`	⬇️
...ia/internal/common/stream/ByteBufDecoderInput.java	`84.89% <0.00%> (-2.16%)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 829992a...353c02d. Read the comment docs.

minwoox

Thanks, @ikhoon!

jrhee17

Looks great! Thank you @ikhoon 🙇 👍 🙇

jrhee17 · 2022-03-04T09:36:56Z

core/src/main/java/com/linecorp/armeria/server/HttpResponseException.java

+ * <p>Note that an {@link HttpResponseException} raised may not be applied to the next decorators if the
+ * {@link HttpResponseException} is not recovered before passed to the next decorator chain.
+ * For that reason, you need to properly handle the thrown {@link HttpResponse} into a normal


since raised is describing the exception, HttpResponse -> HttpResponseException, better not to use pronouns "you", "I" in academic writing

Suggested change

* <p>Note that an {@link HttpResponseException} raised may not be applied to the next decorators if the

* {@link HttpResponseException} is not recovered before passed to the next decorator chain.

* For that reason, you need to properly handle the thrown {@link HttpResponse} into a normal

* <p>Note that a raised {@link HttpResponseException} may not be applied to the next decorators if the

* {@link HttpResponseException} is not recovered before being passed to the next decorator chain.

* For this reason, the thrown {@link HttpResponseException} should be converted into a normal

jrhee17 · 2022-03-07T06:49:15Z

thrift0.13/src/test/java/com/linecorp/armeria/it/thrift/ThriftHttpErrorResponseTest.java

@@ -40,23 +44,41 @@
 /**
 * Tests if Armeria decorators can alter the request/response timeout specified in Thrift call parameters.
 */
-public class ThriftHttpErrorResponseTest {
+class ThriftHttpErrorResponseTest {


👍👍👍

jrhee17 · 2022-03-07T06:50:37Z

core/src/main/java/com/linecorp/armeria/server/HttpStatusException.java

+ * <p>Note that an {@link HttpStatusException} raised may not be applied to the next decorators if the
+ * {@link HttpStatusException} is not recovered before passed to the next decorator chain.
+ * For that reason, you need to properly handle the thrown {@link HttpStatus} into a normal


since raised is describing the exception, HttpStatus -> HttpStatusException, better not to use pronouns "you", "I" in academic writing

Suggested change

* <p>Note that an {@link HttpStatusException} raised may not be applied to the next decorators if the

* {@link HttpStatusException} is not recovered before passed to the next decorator chain.

* For that reason, you need to properly handle the thrown {@link HttpStatus} into a normal

* <p>Note that a raised {@link HttpStatusException} may not be applied to the next decorators if the

* {@link HttpStatusException} is not recovered before being passed to the next decorator chain.

* For this reason, the thrown {@link HttpStatusException} should be converted into a normal

TIL. ❤️

jrhee17 · 2022-03-07T07:16:26Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java

+        return HttpResponse.from(updateHandler.handle(ctx, req).handle((updateResult, cause) -> {
+            if (cause != null) {
+                cause = Exceptions.peel(cause);
+                return HttpResponse.ofFailure(cause);


Q) I understand that even with this change users won't be able to mutate response headers fluently from decorators - I want to make sure this was intended (no problem since I don't think the behavior is changed)

Ping @ikhoon

It looks cruft. Let me revert this change. 😅

trustin · 2022-03-14T10:41:24Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/DefaultHealthCheckUpdateHandler.java

@@ -50,7 +51,7 @@
            case PATCH:
                return req.aggregate().thenApply(DefaultHealthCheckUpdateHandler::handlePatch);
            default:
-                throw HttpStatusException.of(HttpStatus.METHOD_NOT_ALLOWED);
+                return exceptionallyCompletedFuture(HttpStatusException.of(HttpStatus.METHOD_NOT_ALLOWED));


nit: Could use UnmodifiableFuture.exceptionallyCompletedFuture()

trustin · 2022-03-14T10:45:15Z

core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java

+        return HttpResponse.from(updateHandler.handle(ctx, req).handle((updateResult, cause) -> {
+            if (cause != null) {
+                cause = Exceptions.peel(cause);
+                return HttpResponse.ofFailure(cause);


Ping @ikhoon

trustin

Thanks, @ikhoon !

Motivation: An `HttpResponseException` has an `HttpResponse`. The thrown response is extracted by the default `ServerErrorHandler` and converted to a normal `HttpResponse`. There are downsides to `HttpResponseException`. As the response is wrapped by `HttpResponseException`, decorators can not access the `HttpObject`s of the response in the exception using `mapXXX`. Users may find it hard to mutate the thrown `HttpResponse` because recovery should be done first. Therefore, it would be better not to use `HttpResponseException` internally or recover it before passing the response to decorators so that users use `HttpResponse.mapXXX` to transform a returned response in decorators. Modifications: - Add `HttpFile.ofRedirect(location)` to indicate a file in a different location. - Recover an `Http{Response,Status}Exception` thrown by `HealthCheckUpdateHandler` - Update Javadoc in detail for `Http{Response,Status}Exception` - Correctly propagate the cause of `Http{Response,Status}Exception` in `THttpService` Result: - You can now mutate a redirect response from `FileService` using `mapHeaders` - You no longer see an `HttpResponseException` or an `HttpStatusException` from built-in services. - Fixes line#4056

Motivation: We decided not to use `HttpStatusException` and `HttpResponseException` internally because if an exception is thrown, it break through decorator chains and we can't use stream operatorators in `HttpResponse`. line#4117 Thad said, `FallbackService` still throws defered `HttpStatusException`s which we should've migrated in line#4117 but omitted it. Modification: - Return a normal `HttpResponse` instead of throwing `HttpStatusException` in `FallbackService`. Result: You no longer see `HttpStatusException` when there is no matching route.

Motivation: We decided not to use `HttpStatusException` and `HttpResponseException` internally because if an exception is thrown, it breaks through decorator chains and we can't use stream operators in `HttpResponse`. #4117 Thad said, `FallbackService` still throws deferred `HttpStatusException`s which we should've migrated in #4117 but omitted it. Fixing the bug, I found out that there is a race condition between `FallbackService` and `Http*RequestDecoder`. If a request exceeds the maximum length limit, `Http*RequestDecoder` will return 413 Request Entity Too Large to the request. If a matching route is not found, `FallbackService` returns 404 Not Found. Armeria tests expect 404 for that case. https://github.com/line/armeria/blob/be306ab4c9de5386e74999baec5ebaae82618b91/core/src/test/java/com/linecorp/armeria/server/HttpServerTest.java#L598-L605 It was possible since `FallbackService` threw an exception that aborts a request immediately. https://github.com/line/armeria/blob/8aebaf475eeee1ee3fec6c7c876456878a8b423e/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java#L365-L372 If `FallbackService` returns a normal response, the request will be closed when a response is written completely. https://github.com/line/armeria/blob/8aebaf475eeee1ee3fec6c7c876456878a8b423e/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java#L422-L429 As a result, `Http*RequestDecoder` can return a 413 response to the request being handled by `FallbackService`. Modification: - Return a normal `HttpResponse` instead of throwing `HttpStatusException` in `FallbackService`. - A headers-only response is used to finish the response quickly before `Http*RequestDecoder` receives additional messages. - Fixed `Http1RequestDecoder.fail()` to check if a response to the request was sent before to avoid duplicate responses. Result: - You no longer see `HttpStatusException` when there is no matching route. - Fixes #4548

ikhoon added the defect label Feb 25, 2022

ikhoon added this to the 1.15.0 milestone Feb 25, 2022

ikhoon requested review from jrhee17, minwoox and trustin as code owners February 25, 2022 12:56

Peel exception

4599cfd

minwoox approved these changes Feb 28, 2022

View reviewed changes

jrhee17 approved these changes Mar 7, 2022

View reviewed changes

jrhee17 reviewed Mar 7, 2022

View reviewed changes

Address comments by @jrhee17

2c1c181

trustin reviewed Mar 14, 2022

View reviewed changes

Address comments

353c02d

trustin approved these changes Mar 22, 2022

View reviewed changes

trustin merged commit 43c63ea into line:master Mar 22, 2022

ikhoon deleted the httpfile.redirect branch March 28, 2022 05:53

minwoox mentioned this pull request Jul 28, 2022

Support file upload for GraphQL #4199

Merged

ikhoon mentioned this pull request Nov 25, 2022

Fix FallbackService not to raise HttpStatusException #4552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix not to use `Http{Response,Status}Exception` internally #4117

Fix not to use `Http{Response,Status}Exception` internally #4117

ikhoon commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 28, 2022 •

edited

Loading

minwoox left a comment

jrhee17 left a comment

jrhee17 Mar 4, 2022

jrhee17 Mar 7, 2022

jrhee17 Mar 7, 2022

ikhoon Mar 11, 2022

jrhee17 Mar 7, 2022

trustin Mar 14, 2022

ikhoon Mar 15, 2022

trustin Mar 14, 2022

trustin Mar 14, 2022

trustin left a comment

Fix not to use Http{Response,Status}Exception internally #4117

Fix not to use Http{Response,Status}Exception internally #4117

Conversation

ikhoon commented Feb 25, 2022 • edited Loading

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Fix not to use `Http{Response,Status}Exception` internally #4117

Fix not to use `Http{Response,Status}Exception` internally #4117

ikhoon commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 28, 2022 •

edited

Loading