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.

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

Jump to bottom

Provide RequestHeaders to ServerErrorHandler #4037

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Provide RequestHeaders to ServerErrorHandler #4037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c3dd01
Provide `RequestHeaders` to `ServerErrorHandler`
trustin Jan 20, 2022
3b18417
Fix test failures
trustin Jan 20, 2022
12626aa
Remove unnecessary `@Nullable`
trustin Jan 24, 2022
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
18 changes: 13 additions & 5 deletions core/src/main/java/com/linecorp/armeria/server/DefaultServerErrorHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.MediaType;
import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.armeria.common.ResponseHeaders;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.Exceptions;
Expand Down Expand Up @@ -68,7 +69,8 @@ public HttpResponse onServiceException(ServiceRequestContext ctx, Throwable caus
logger.warn("{} Failed processing a request:", ctx, cause);
}

return internalRenderStatus(serviceConfig, HttpStatus.BAD_REQUEST, cause);
return internalRenderStatus(serviceConfig, ctx.request().headers(),
HttpStatus.BAD_REQUEST, cause);
}
}

Expand All @@ -85,33 +87,39 @@ public HttpResponse onServiceException(ServiceRequestContext ctx, Throwable caus
}

if (cause instanceof RequestTimeoutException) {
return internalRenderStatus(serviceConfig, HttpStatus.SERVICE_UNAVAILABLE, cause);
return internalRenderStatus(serviceConfig, ctx.request().headers(),
HttpStatus.SERVICE_UNAVAILABLE, cause);
}

if (isAnnotatedService && needsToWarn() && !Exceptions.isExpected(cause)) {
logger.warn("{} Unhandled exception from a service:", ctx, cause);
}

return internalRenderStatus(serviceConfig, HttpStatus.INTERNAL_SERVER_ERROR, cause);
return internalRenderStatus(serviceConfig, ctx.request().headers(),
HttpStatus.INTERNAL_SERVER_ERROR, cause);
}

@SuppressWarnings("deprecation")
private static boolean needsToWarn() {
return Flags.annotatedServiceExceptionVerbosity() == ExceptionVerbosity.UNHANDLED &&
logger.isWarnEnabled();
}

private static HttpResponse internalRenderStatus(ServiceConfig serviceConfig,
RequestHeaders headers,
HttpStatus status,
@Nullable Throwable cause) {
final AggregatedHttpResponse res = serviceConfig.server().config().errorHandler()
.renderStatus(serviceConfig, status, null, cause);
final AggregatedHttpResponse res =
serviceConfig.server().config().errorHandler()
.renderStatus(serviceConfig, headers, status, null, cause);
assert res != null;
return res.toHttpResponse();
}

@Nonnull
@Override
public AggregatedHttpResponse renderStatus(ServiceConfig config,
@Nullable RequestHeaders headers,
HttpStatus status,
@Nullable String description,
@Nullable Throwable cause) {
Expand Down
87 changes: 50 additions & 37 deletions core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,33 +145,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
keepAliveHandler.increaseNumRequests();
final HttpRequest nettyReq = (HttpRequest) msg;
if (!nettyReq.decoderResult().isSuccess()) {
fail(id, HttpStatus.BAD_REQUEST, "Decoder failure", null);
fail(id, null, HttpStatus.BAD_REQUEST, "Decoder failure", null);
return;
}

final HttpHeaders nettyHeaders = nettyReq.headers();

// Do not accept unsupported methods.
final io.netty.handler.codec.http.HttpMethod nettyMethod = nettyReq.method();
if (nettyMethod == io.netty.handler.codec.http.HttpMethod.CONNECT ||
!HttpMethod.isSupported(nettyMethod.name())) {
fail(id, HttpStatus.METHOD_NOT_ALLOWED, "Unsupported method", null);
if (!HttpMethod.isSupported(nettyMethod.name())) {
fail(id, null, HttpStatus.METHOD_NOT_ALLOWED, "Unsupported method", null);
return;
}

// Handle `expect: 100-continue` first to give `handle100Continue()` a chance to remove
// the `expect` header before converting the Netty HttpHeaders into Armeria RequestHeaders.
// This is because removing a header from RequestHeaders is more expensive due to its
// immutability.
final boolean hasInvalidExpectHeader = !handle100Continue(id, nettyReq);

// Convert the Netty HttpHeaders into Armeria RequestHeaders.
final RequestHeaders headers =
ArmeriaHttpUtil.toArmeria(ctx, nettyReq, cfg, scheme.toString());

// Do not accept a CONNECT request.
if (headers.method() == HttpMethod.CONNECT) {
fail(id, headers, HttpStatus.METHOD_NOT_ALLOWED, "Unsupported method", null);
return;
}

// Validate the 'content-length' header.
final String contentLengthStr = nettyHeaders.get(HttpHeaderNames.CONTENT_LENGTH);
final String contentLengthStr = headers.get(HttpHeaderNames.CONTENT_LENGTH);
final boolean contentEmpty;
if (contentLengthStr != null) {
final long contentLength;
long contentLength;
try {
contentLength = Long.parseLong(contentLengthStr);
} catch (NumberFormatException ignored) {
fail(id, HttpStatus.BAD_REQUEST, "Invalid content length", null);
return;
contentLength = -1;
}
if (contentLength < 0) {
fail(id, HttpStatus.BAD_REQUEST, "Invalid content length", null);
fail(id, headers, HttpStatus.BAD_REQUEST, "Invalid content length", null);
return;
}

Expand All @@ -180,32 +192,30 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
contentEmpty = true;
}

if (!handle100Continue(id, nettyReq, nettyHeaders)) {
// Reject the requests with an `expect` header whose value is not `100-continue`.
if (hasInvalidExpectHeader) {
ctx.pipeline().fireUserEventTriggered(HttpExpectationFailedEvent.INSTANCE);
fail(id, HttpStatus.EXPECTATION_FAILED, null, null);
fail(id, headers, HttpStatus.EXPECTATION_FAILED, null, null);
return;
}

// Close the request early when it is sure that there will be
// neither content nor trailers.
// Close the request early when it is certain there will be neither content nor trailers.
final EventLoop eventLoop = ctx.channel().eventLoop();
final RequestHeaders armeriaRequestHeaders =
ArmeriaHttpUtil.toArmeria(ctx, nettyReq, cfg, scheme.toString());
final boolean keepAlive = HttpUtil.isKeepAlive(nettyReq);
if (contentEmpty && !HttpUtil.isTransferEncodingChunked(nettyReq)) {
this.req = req = new EmptyContentDecodedHttpRequest(
eventLoop, id, 1, armeriaRequestHeaders, keepAlive);
eventLoop, id, 1, headers, keepAlive);
} else {
this.req = req = new DefaultDecodedHttpRequest(
eventLoop, id, 1, armeriaRequestHeaders, keepAlive, inboundTrafficController,
eventLoop, id, 1, headers, keepAlive, inboundTrafficController,
// FIXME(trustin): Use a different maxRequestLength for a different virtual
// host.
cfg.defaultVirtualHost().maxRequestLength());
}

ctx.fireChannelRead(req);
} else {
fail(id, HttpStatus.BAD_REQUEST, "Invalid decoder state", null);
fail(id, null, HttpStatus.BAD_REQUEST, "Invalid decoder state", null);
return;
}
}
Expand All @@ -223,11 +233,11 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
final HttpContent content = (HttpContent) msg;
final DecoderResult decoderResult = content.decoderResult();
if (!decoderResult.isSuccess()) {
final HttpStatus badRequest = HttpStatus.BAD_REQUEST;
fail(id, badRequest, Http2Error.PROTOCOL_ERROR, "Decoder failure", null);
fail(id, decodedReq.headers(), HttpStatus.BAD_REQUEST,
Http2Error.PROTOCOL_ERROR, "Decoder failure", null);
final ProtocolViolationException cause =
new ProtocolViolationException(decoderResult.cause());
decodedReq.close(HttpStatusException.of(badRequest, cause));
decodedReq.close(HttpStatusException.of(HttpStatus.BAD_REQUEST, cause));
return;
}

Expand All @@ -244,11 +254,11 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
.contentLength(req.headers())
.transferred(transferredLength)
.build();
final HttpStatus entityTooLarge = HttpStatus.REQUEST_ENTITY_TOO_LARGE;
fail(id, entityTooLarge, Http2Error.CANCEL, null, cause);
fail(id, decodedReq.headers(), HttpStatus.REQUEST_ENTITY_TOO_LARGE,
Http2Error.CANCEL, null, cause);
// Wrap the cause with the returned status to let LoggingService correctly log the
// status.
decodedReq.close(HttpStatusException.of(entityTooLarge, cause));
decodedReq.close(HttpStatusException.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, cause));
return;
}

Expand All @@ -268,25 +278,27 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
}
}
} catch (URISyntaxException e) {
final HttpStatus badRequest = HttpStatus.BAD_REQUEST;
fail(id, badRequest, Http2Error.CANCEL, "Invalid request path", e);
if (req != null) {
req.close(HttpStatusException.of(badRequest, e));
fail(id, req.headers(), HttpStatus.BAD_REQUEST, Http2Error.CANCEL, "Invalid request path", e);
req.close(HttpStatusException.of(HttpStatus.BAD_REQUEST, e));
} else {
fail(id, null, HttpStatus.BAD_REQUEST, Http2Error.CANCEL, "Invalid request path", e);
}
} catch (Throwable t) {
final HttpStatus serverError = HttpStatus.INTERNAL_SERVER_ERROR;
fail(id, serverError, Http2Error.INTERNAL_ERROR, null, t);
if (req != null) {
req.close(HttpStatusException.of(serverError, t));
fail(id, req.headers(), HttpStatus.INTERNAL_SERVER_ERROR, Http2Error.INTERNAL_ERROR, null, t);
req.close(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR, t));
} else {
fail(id, null, HttpStatus.INTERNAL_SERVER_ERROR, Http2Error.INTERNAL_ERROR, null, t);
logger.warn("Unexpected exception:", t);
}
} finally {
ReferenceCountUtil.release(msg);
}
}

private boolean handle100Continue(int id, HttpRequest nettyReq, HttpHeaders nettyHeaders) {
private boolean handle100Continue(int id, HttpRequest nettyReq) {
final HttpHeaders nettyHeaders = nettyReq.headers();
if (nettyReq.protocolVersion().compareTo(HttpVersion.HTTP_1_1) < 0) {
// Ignore HTTP/1.0 requests.
return true;
Expand All @@ -311,24 +323,25 @@ private boolean handle100Continue(int id, HttpRequest nettyReq, HttpHeaders nett
return true;
}

private void fail(int id, HttpStatus status, Http2Error error,
private void fail(int id, @Nullable RequestHeaders headers, HttpStatus status, Http2Error error,
@Nullable String message, @Nullable Throwable cause) {
if (encoder.isResponseHeadersSent(id, 1)) {
// The response is sent or being sent by HttpResponseSubscriber so we cannot send
// the error response.
encoder.writeReset(id, 1, error);
} else {
fail(id, status, message, cause);
fail(id, headers, status, message, cause);
}
}

private void fail(int id, HttpStatus status, @Nullable String message, @Nullable Throwable cause) {
private void fail(int id, @Nullable RequestHeaders headers, HttpStatus status,
@Nullable String message, @Nullable Throwable cause) {
discarding = true;
req = null;

// FIXME(trustin): Use a different verboseResponses for a different virtual host.
encoder.writeErrorResponse(id, 1, cfg.defaultVirtualHost().fallbackServiceConfig(),
status, message, cause);
headers, status, message, cause);
}

@Override
Expand Down
Loading