Skip to content

Add decorator support for ServiceErrorHandler #6570

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.

Sign up for GitHub

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

Open
0x1306e6d wants to merge 3 commits into
base: main
Choose a base branch
from
+235 −94
Open

Add decorator support for ServiceErrorHandler #6570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69ba77d
Add decorator support for ServiceErrorHandler
0x1306e6d Dec 30, 2025
d417af7
Address comments by @jrhee17 and @minwoox
0x1306e6d Dec 31, 2025
3e0951b
Address comments by @minwoox
0x1306e6d Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions ...ver/ContentPreviewServerErrorHandler.java → ...ia/server/ContentPreviewErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,11 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.server.logging.ContentPreviewingService;

final class ContentPreviewServerErrorHandler implements DecoratingServerErrorHandlerFunction {
final class ContentPreviewErrorHandler implements DecoratingErrorHandlerFunction {

@Nullable
@Override
public HttpResponse onServiceException(ServerErrorHandler delegate,
ServiceRequestContext ctx, Throwable cause) {
final HttpResponse res = delegate.onServiceException(ctx, cause);

private static HttpResponse maybeSetUpResponseContentPreviewer(ServiceRequestContext ctx,
@Nullable HttpResponse res) {
final ContentPreviewingService contentPreviewingService =
ctx.findService(ContentPreviewingService.class);
if (contentPreviewingService == null) {
Expand All @@ -45,4 +42,20 @@ public HttpResponse onServiceException(ServerErrorHandler delegate,
ctx.logBuilder().responseContentPreview(null);
return res;
}

@Nullable
@Override
public HttpResponse onServiceException(ServerErrorHandler delegate,
ServiceRequestContext ctx, Throwable cause) {
final HttpResponse res = delegate.onServiceException(ctx, cause);
return maybeSetUpResponseContentPreviewer(ctx, res);
}

@Nullable
@Override
public HttpResponse onServiceException(ServiceErrorHandler delegate,
ServiceRequestContext ctx, Throwable cause) {
final HttpResponse res = delegate.onServiceException(ctx, cause);
return maybeSetUpResponseContentPreviewer(ctx, res);
}
}
30 changes: 21 additions & 9 deletions ...rmeria/server/CorsServerErrorHandler.java → ...corp/armeria/server/CorsErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,19 @@
import com.linecorp.armeria.server.cors.CorsService;

/**
* wraps ServerErrorHandler for adding CORS headers to error responses.
* A {@link DecoratingErrorHandlerFunction} for adding CORS headers to error responses.
*/
final class CorsServerErrorHandler implements DecoratingServerErrorHandlerFunction {
final class CorsErrorHandler implements DecoratingErrorHandlerFunction {

private static void maybeSetCorsHeaders(ServiceRequestContext ctx) {
final CorsService corsService = ctx.findService(CorsService.class);
if (shouldSetCorsHeaders(corsService, ctx)) {
assert corsService != null;
ctx.mutateAdditionalResponseHeaders(builder -> {
CorsHeaderUtil.setCorsResponseHeaders(ctx, ctx.request(), builder, corsService.config());
});
}
}

/**
* Sets CORS headers for <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests">
Expand Down Expand Up @@ -60,13 +70,15 @@ private static boolean shouldSetCorsHeaders(@Nullable CorsService corsService, S
@Override
public HttpResponse onServiceException(ServerErrorHandler delegate,
ServiceRequestContext ctx, Throwable cause) {
final CorsService corsService = ctx.findService(CorsService.class);
if (shouldSetCorsHeaders(corsService, ctx)) {
assert corsService != null;
ctx.mutateAdditionalResponseHeaders(builder -> {
CorsHeaderUtil.setCorsResponseHeaders(ctx, ctx.request(), builder, corsService.config());
});
}
maybeSetCorsHeaders(ctx);
return delegate.onServiceException(ctx, cause);
}

@Nullable
@Override
public HttpResponse onServiceException(ServiceErrorHandler delegate,
ServiceRequestContext ctx, Throwable cause) {
maybeSetCorsHeaders(ctx);
return delegate.onServiceException(ctx, cause);
}
}
11 changes: 8 additions & 3 deletions ...DecoratingServerErrorHandlerFunction.java → ...erver/DecoratingErrorHandlerFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,15 @@
import com.linecorp.armeria.common.annotation.Nullable;

/**
* A function to decorate a {@link ServerErrorHandler} with additional behavior. It is typically used to inject
* additional logic before or after the specified {@link ServerErrorHandler} execution.
* A function to decorate a {@link ServerErrorHandler} or a {@link ServiceErrorHandler} with additional
* behavior. Implementations can wrap error handlers to add pre-processing, post-processing, or
* conditional logic around error handling. The decorator receives the delegate error handler and
* can decide whether and when to invoke it.
*/
interface DecoratingServerErrorHandlerFunction {
interface DecoratingErrorHandlerFunction {
@Nullable
HttpResponse onServiceException(ServerErrorHandler delegate, ServiceRequestContext ctx, Throwable cause);

@Nullable
HttpResponse onServiceException(ServiceErrorHandler delegate, ServiceRequestContext ctx, Throwable cause);
}
52 changes: 44 additions & 8 deletions .../server/ServerErrorHandlerDecorators.java → ...rmeria/server/ErrorHandlerDecorators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,29 +26,37 @@
import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.armeria.common.annotation.Nullable;

final class ServerErrorHandlerDecorators {
final class ErrorHandlerDecorators {

private static final List<DecoratingServerErrorHandlerFunction> decoratorFunctions =
ImmutableList.of(new CorsServerErrorHandler(),
new ContentPreviewServerErrorHandler());
private static final List<DecoratingErrorHandlerFunction> decoratorFunctions =
ImmutableList.of(new CorsErrorHandler(),
new ContentPreviewErrorHandler());

private ServerErrorHandlerDecorators() {}
private ErrorHandlerDecorators() {}

static ServerErrorHandler decorate(ServerErrorHandler delegate) {
ServerErrorHandler decorated = delegate;
for (DecoratingServerErrorHandlerFunction decoratorFunction : decoratorFunctions) {
for (DecoratingErrorHandlerFunction decoratorFunction : decoratorFunctions) {
Copy link
Contributor

@jrhee17 jrhee17 Dec 31, 2025

Choose a reason for hiding this comment

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

Question) Since the ServiceErrorHandler is decorated, is there still a need to decorate the ServerErrorHandler?

e.g. the following diff seems to still pass the test:

user@AL02437565 upstream-armeria % git diff
diff --git a/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java b/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
index faa2867cf..a614d3af0 100644
--- a/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
+++ b/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
@@ -35,11 +35,7 @@ final class ErrorHandlerDecorators {
     private ErrorHandlerDecorators() {}
 
     static ServerErrorHandler decorate(ServerErrorHandler delegate) {
-        ServerErrorHandler decorated = delegate;
-        for (DecoratingErrorHandlerFunction decoratorFunction : decoratorFunctions) {
-            decorated = new DecoratingServerErrorHandler(decorated, decoratorFunction);
-        }
-        return decorated;
+        return delegate;
     }
 
     static ServiceErrorHandler decorate(ServiceErrorHandler delegate) {
user@AL02437565 upstream-armeria %

The concern is that implementors of the default ErrorHandlerDecorators#decoratorFunctions need to be aware that each DecoratingErrorHandlerFunction may be applied twice.

Copy link
Contributor

@minwoox minwoox Dec 31, 2025

Choose a reason for hiding this comment

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

That's a good finding. I think we need to remove the decorating here:

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

Line 2338 in 69ba77d

final ServerErrorHandler errorHandler = ErrorHandlerDecorators.decorate(

and add it here instead:

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

Line 261 in 603fc86

this.errorHandler = requireNonNull(errorHandler, "errorHandler");

Copy link
Contributor Author

@0x1306e6d 0x1306e6d Dec 31, 2025

Choose a reason for hiding this comment

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

Thanks!

Regardless, I found another edge case where the content preview is not recorded. An HttpResponse can be created directly via renderStatus(...) when it is an AbortedHttpResponse. If a service returns HttpResponse.ofFailure(HttpResponseException(...)) without an error handler, the error response is created using renderStatus(...) directly:

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

Lines 275 to 278 in 897b5a0

} else if (peeled instanceof HttpStatusException) {
final Throwable cause0 = firstNonNull(peeled.getCause(), peeled);
final AggregatedHttpResponse res = toAggregatedHttpResponse((HttpStatusException) peeled);
failAndRespond(cause0, res, Http2Error.CANCEL, false);

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

Lines 202 to 211 in 897b5a0

final AggregatedHttpResponse toAggregatedHttpResponse(HttpStatusException cause) {
final HttpStatus status = cause.httpStatus();
final Throwable cause0 = firstNonNull(cause.getCause(), cause);
final ServiceConfig serviceConfig = reqCtx.config();
final AggregatedHttpResponse response = serviceConfig.errorHandler()
.renderStatus(reqCtx, req.headers(), status,
null, cause0);
assert response != null;
return response;
}

Because onServiceException(...) is not called with the returned response, the request log does not complete. My idea is to apply the same decorating logic to the response returned from the delegate’s renderStatus(...).

I’m unsure whether there is any chance the decorator could be applied twice. For example, via renderStatus(...) -> FallbackService -> ContentPreviewingService or CorsService.

Copy link
Contributor

@minwoox minwoox Jan 2, 2026

Choose a reason for hiding this comment

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

Good catch, there's definitely an edge case here.
Plus, it looks like we're generating a content preview even when a preceding decorator fails, which we shouldn't be doing.
What do you think about preventing the preview from being generated whenever an exception is raised?
Since we already set the preview to null on an exception in DefaultRequestLog, this seems like the right fix.

armeria/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java

Lines 1471 to 1473 in d068172

// Will auto-fill response content and its preview if response has failed.
deferredFlags = this.deferredFlags & ~(RESPONSE_CONTENT.flag() |
RESPONSE_CONTENT_PREVIEW.flag());

I think we could set the preview to null at these two spots to solve it:

Copy link
Contributor Author

@0x1306e6d 0x1306e6d Jan 2, 2026

Choose a reason for hiding this comment

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

Plus, it looks like we're generating a content preview even when a preceding decorator fails, which we shouldn't be doing.

Indeed, because content preview generation is done after the response is processed by the recovery step and whether the content preview decorator was actually executed is not considered.

What do you think about preventing the preview from being generated whenever an exception is raised?

If so, should we generate the preview for an aborted response (HttpResponse.ofFailure(...))? I’m a bit confused about what ContentPreviewErrorHandler would do if we choose this behavior.

Copy link
Contributor Author

@0x1306e6d 0x1306e6d Jan 6, 2026

Choose a reason for hiding this comment

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

Glad to hear that. I agree that #6577 is the ultimate goal for addressing all of these issues.

Copy link
Contributor

@minwoox minwoox Jan 7, 2026

Choose a reason for hiding this comment

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

@0x1306e6d So, could you fix this PR to always complete the request log?

Copy link
Contributor Author

@0x1306e6d 0x1306e6d Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I don't fully understand what 'always complete the request log' means. Could you please provide more details? If you meant setting the content preview to null for failed responses, I’m unsure how to retrieve the failed response preview until #6577 is available.

Copy link
Contributor

@minwoox minwoox Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If you meant setting the content preview to null for failed responses,

Yeah, that's what I meant. 😉
I initially thought we should prioritize the completeness of the request log, even if it meant disabling the content preview on failed responses. The idea was to ship this fix before #6577.
However, we already released 1.35.0 and Since #6577 will likely be included in the next release, we can probably wait for that.

So if you don't want to apply the workaround for this PR, I think we can just close this PR and implement #6577.

Copy link
Contributor Author

@0x1306e6d 0x1306e6d Jan 9, 2026

Choose a reason for hiding this comment

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

I see. I added a commit to ensure the termination of RequestLog, and #6577 will provide the fundamental fix for the content preview.

One question I have is that the RequestLog termination may have different characteristics from the original PR. Also, CorsService requires a ServiceErrorHandler decorator. Shouldn’t we separate these into different PRs for better management?

decorated = new DecoratingServerErrorHandler(decorated, decoratorFunction);
}
return decorated;
}

static ServiceErrorHandler decorate(ServiceErrorHandler delegate) {
ServiceErrorHandler decorated = delegate;
for (DecoratingErrorHandlerFunction decoratorFunction : decoratorFunctions) {
decorated = new DecoratingServiceErrorHandler(decorated, decoratorFunction);
}
return decorated;
}

private static final class DecoratingServerErrorHandler implements ServerErrorHandler {

final ServerErrorHandler delegate;
final DecoratingServerErrorHandlerFunction decoratorFunction;
final DecoratingErrorHandlerFunction decoratorFunction;

DecoratingServerErrorHandler(ServerErrorHandler delegate,
DecoratingServerErrorHandlerFunction decoratorFunction) {
DecoratingErrorHandlerFunction decoratorFunction) {
this.delegate = delegate;
this.decoratorFunction = decoratorFunction;
}
Expand Down Expand Up @@ -79,4 +87,32 @@ public AggregatedHttpResponse renderStatus(@Nullable ServiceRequestContext ctx,
return delegate.renderStatus(ctx, config, headers, status, description, cause);
}
}

private static final class DecoratingServiceErrorHandler implements ServiceErrorHandler {

final ServiceErrorHandler delegate;
final DecoratingErrorHandlerFunction decoratorFunction;

DecoratingServiceErrorHandler(ServiceErrorHandler delegate,
DecoratingErrorHandlerFunction decoratorFunction) {
this.delegate = delegate;
this.decoratorFunction = decoratorFunction;
}

@Nullable
@Override
public HttpResponse onServiceException(ServiceRequestContext ctx, Throwable cause) {
return decoratorFunction.onServiceException(delegate, ctx, cause);
}

@Nullable
@Override
public AggregatedHttpResponse renderStatus(ServiceRequestContext ctx,
RequestHeaders headers,
HttpStatus status,
@Nullable String description,
@Nullable Throwable cause) {
return delegate.renderStatus(ctx, headers, status, description, cause);
}
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2335,7 +2335,7 @@ DefaultServerConfig buildServerConfig(List<ServerPort> serverPorts) {
shutdownSupports.add(ShutdownSupport.of(tlsProvider));
}

final ServerErrorHandler errorHandler = ServerErrorHandlerDecorators.decorate(
final ServerErrorHandler errorHandler = ErrorHandlerDecorators.decorate(
this.errorHandler == null ? ServerErrorHandler.ofDefault()
: this.errorHandler.orElse(ServerErrorHandler.ofDefault()));
final MeterIdPrefix meterIdPrefix = tlsConfig != null ? tlsConfig.meterIdPrefix() : null;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,9 @@ ServiceConfig build(ServiceNaming defaultServiceNaming,
ServiceErrorHandler defaultServiceErrorHandler,
@Nullable UnloggedExceptionsReporter unloggedExceptionsReporter,
String baseContextPath, Supplier<AutoCloseable> contextHook) {
ServiceErrorHandler errorHandler =
ServiceErrorHandler errorHandler = ErrorHandlerDecorators.decorate(
serviceErrorHandler != null ? serviceErrorHandler.orElse(defaultServiceErrorHandler)
: defaultServiceErrorHandler;
: defaultServiceErrorHandler);
if (unloggedExceptionsReporter != null) {
errorHandler = new ExceptionReportingServiceErrorHandler(errorHandler,
unloggedExceptionsReporter);
Expand Down
39 changes: 38 additions & 1 deletion ...src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.linecorp.armeria.common.logging.RequestLog;
import com.linecorp.armeria.server.HttpStatusException;
import com.linecorp.armeria.server.ServerBuilder;
import com.linecorp.armeria.server.ServiceErrorHandler;
import com.linecorp.armeria.server.annotation.ExceptionHandlerFunction;
import com.linecorp.armeria.server.annotation.Get;
import com.linecorp.armeria.server.annotation.ProducesText;
Expand Down Expand Up @@ -69,6 +70,34 @@ protected void configure(ServerBuilder sb) {
return HttpResponse.of("exceptionHandler: " + cause.getMessage());
});

final ServiceErrorHandler serviceErrorHandler = (ctx, cause) -> {
return HttpResponse.of("serviceErrorHandler: " + cause.getMessage());
};
sb.route()
.path("/binding/aborted/http-status-exception")
.errorHandler(serviceErrorHandler)
.build((ctx, req) -> {
return HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR));
});
sb.route()
.path("/binding/aborted/unexpected-exception")
.errorHandler(serviceErrorHandler)
.build((ctx, req) -> {
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
});
sb.route()
.path("/binding/throw/http-status-exception")
.errorHandler(serviceErrorHandler)
.build((ctx, req) -> {
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
});
sb.route()
.path("/binding/throw/unexpected-exception")
.errorHandler(serviceErrorHandler)
.build((ctx, req) -> {
throw new IllegalStateException("Oops!");
});

sb.decorator(LoggingService.newDecorator());
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
}
Expand Down Expand Up @@ -137,7 +166,15 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
Arguments.of("/annotatedExceptionHandler/throw/http-status-exception",
"exceptionHandler: 500 Internal Server Error"),
Arguments.of("/annotatedExceptionHandler/throw/unexpected-exception",
"exceptionHandler: Oops!"));
"exceptionHandler: Oops!"),
Arguments.of("/binding/aborted/http-status-exception",
"serviceErrorHandler: 500 Internal Server Error"),
Arguments.of("/binding/aborted/unexpected-exception",
"serviceErrorHandler: Oops!"),
Arguments.of("/binding/throw/http-status-exception",
"serviceErrorHandler: 500 Internal Server Error"),
Arguments.of("/binding/throw/unexpected-exception",
"serviceErrorHandler: Oops!"));
}
}
}
Loading