Skip to content

Commit

Permalink
[fix] ConjureExceptionHandler understands QosExceptions (#219)
Browse files Browse the repository at this point in the history 
## Before this PR

There are three kinds of QosException in conjure-java-runtime-api, but the ConjureExceptionHandler currently sends them all over the wire as 500 errors.

(See the full list of available [exceptions here](https://github.com/palantir/conjure-java-runtime-api/tree/develop/errors/src/main/java/com/palantir/conjure/java/api/errors))

## After this PR

The Undertow behaviour matches the old Jersey behaviour ([QosExceptionMapper](https://github.com/palantir/conjure-java-runtime/blob/develop/conjure-java-jersey-server/src/main/java/com/palantir/conjure/java/server/jersey/QosExceptionMapper.java))


<!-- Describe at a high-level why this approach is better. -->

<!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' -->
  • Loading branch information
@iamdanfox @bulldozer-bot
iamdanfox authored and bulldozer-bot[bot] committed Feb 1, 2019
1 parent 7302ebd commit 84cf861
Show file tree
Hide file tree
Showing 2 changed files with 181 additions and 57 deletions.
188 changes: 132 additions & 56 deletions ...ime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.palantir.conjure.java.undertow.runtime;

import com.palantir.conjure.java.api.errors.ErrorType;
import com.palantir.conjure.java.api.errors.QosException;
import com.palantir.conjure.java.api.errors.RemoteException;
import com.palantir.conjure.java.api.errors.SerializableError;
import com.palantir.conjure.java.api.errors.ServiceException;
Expand All @@ -25,8 +26,12 @@
import io.undertow.io.UndertowOutputStream;
import io.undertow.server.HttpHandler;
import io.undertow.server.HttpServerExchange;
import io.undertow.util.Headers;
import java.io.IOException;
import java.io.OutputStream;
import java.time.temporal.ChronoUnit;
import java.util.Optional;
import java.util.function.Consumer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.xnio.IoUtils;
Expand Down Expand Up @@ -60,75 +65,105 @@ public void handleRequest(HttpServerExchange exchange) {
try {
delegate.handleRequest(exchange);
} catch (Throwable throwable) {
final SerializableError error;
final int statusCode;

if (throwable instanceof ServiceException) {
ServiceException exception = (ServiceException) throwable;
statusCode = exception.getErrorType().httpErrorCode();
error = SerializableError.forException(exception);
log(exception);
serviceException(exchange, (ServiceException) throwable);
} else if (throwable instanceof QosException) {
qosException(exchange, (QosException) throwable);
} else if (throwable instanceof RemoteException) {
// RemoteExceptions are thrown by Conjure clients to indicate a remote/service-side problem.
// We forward these exceptions, but change the ErrorType to INTERNAL, i.e., the problem is now
// considered internal to *this* service rather than the originating service. This means in particular
// that Conjure errors are defined only local to a given service and these error types don't
// propagate through other services.
RemoteException exception = (RemoteException) throwable;

// log at WARN instead of ERROR because although this indicates an issue in a remote server
log.warn("Encountered a remote exception. Mapping to an internal error before propagating",
SafeArg.of("errorInstanceId", exception.getError().errorInstanceId()),
SafeArg.of("errorName", exception.getError().errorName()),
SafeArg.of("statusCode", exception.getStatus()),
exception);

ErrorType errorType = ErrorType.INTERNAL;
statusCode = errorType.httpErrorCode();

// Override only the name and code of the error
error = SerializableError.builder()
.from(exception.getError())
.errorName(errorType.name())
.errorCode(errorType.code().toString())
.build();
remoteException(exchange, (RemoteException) throwable);
} else if (throwable instanceof IllegalArgumentException) {
ServiceException exception = new ServiceException(ErrorType.INVALID_ARGUMENT, throwable);
error = SerializableError.forException(exception);
statusCode = exception.getErrorType().httpErrorCode();
log(exception);
illegalArgumentException(exchange, throwable);
} else if (throwable instanceof FrameworkException) {
FrameworkException frameworkException = (FrameworkException) throwable;
statusCode = frameworkException.getStatusCode();
ErrorType errorType = statusCode / 100 == 4 ? ErrorType.INVALID_ARGUMENT : ErrorType.INTERNAL;
ServiceException exception = new ServiceException(errorType, frameworkException);
error = SerializableError.forException(exception);
log(exception);
frameworkException(exchange, (FrameworkException) throwable);
} else if (throwable instanceof Error) {
throw (Error) throwable;
} else {
ServiceException exception = new ServiceException(ErrorType.INTERNAL, throwable);
statusCode = exception.getErrorType().httpErrorCode();
error = SerializableError.forException(exception);
log(exception);
writeResponse(
exchange,
Optional.of(SerializableError.forException(exception)),
exception.getErrorType().httpErrorCode());
}
}
}

// Do not attempt to write the failure if data has already been written
if (!isResponseStarted(exchange)) {
exchange.setStatusCode(statusCode);
try {
serializers.serialize(error, exchange);
} catch (IOException | RuntimeException e) {
log.info("Failed to write error response", e);
private void serviceException(HttpServerExchange exchange, ServiceException exception) {
log(exception);
writeResponse(
exchange,
Optional.of(SerializableError.forException(exception)),
exception.getErrorType().httpErrorCode());
}

private void qosException(HttpServerExchange exchange, QosException exception) {
exception.accept(QOS_EXCEPTION_HEADERS).accept(exchange);
log.debug("Possible quality-of-service intervention", exception);
writeResponse(
exchange,
Optional.empty(),
exception.accept(QOS_EXCEPTION_STATUS_CODE));
}

// RemoteExceptions are thrown by Conjure clients to indicate a remote/service-side problem.
// We forward these exceptions, but change the ErrorType to INTERNAL, i.e., the problem is now
// considered internal to *this* service rather than the originating service. This means in particular
// that Conjure errors are defined only local to a given service and these error types don't
// propagate through other services.
private void remoteException(HttpServerExchange exchange, RemoteException exception) {
// log at WARN instead of ERROR because although this indicates an issue in a remote server
log.warn("Encountered a remote exception. Mapping to an internal error before propagating",
SafeArg.of("errorInstanceId", exception.getError().errorInstanceId()),
SafeArg.of("errorName", exception.getError().errorName()),
SafeArg.of("statusCode", exception.getStatus()),
exception);

ErrorType errorType = ErrorType.INTERNAL;
writeResponse(exchange,
// Override only the name and code of the error
Optional.of(SerializableError.builder()
.from(exception.getError())
.errorName(errorType.name())
.errorCode(errorType.code().toString())
.build()),
errorType.httpErrorCode());
}

private void illegalArgumentException(HttpServerExchange exchange, Throwable throwable) {
ServiceException exception = new ServiceException(ErrorType.INVALID_ARGUMENT, throwable);
log(exception);
writeResponse(
exchange,
Optional.of(SerializableError.forException(exception)),
exception.getErrorType().httpErrorCode());
}

private void frameworkException(HttpServerExchange exchange, FrameworkException frameworkException) {
int statusCode = frameworkException.getStatusCode();
ErrorType errorType = statusCode / 100 == 4 ? ErrorType.INVALID_ARGUMENT : ErrorType.INTERNAL;
ServiceException exception = new ServiceException(errorType, frameworkException);
log(exception);
writeResponse(exchange, Optional.of(SerializableError.forException(exception)), statusCode);
}

private void writeResponse(HttpServerExchange exchange, Optional<SerializableError> maybeBody, int statusCode) {
// Do not attempt to write the failure if data has already been written
if (!isResponseStarted(exchange)) {
exchange.setStatusCode(statusCode);
try {
if (maybeBody.isPresent()) {
serializers.serialize(maybeBody.get(), exchange);
}
} else {
// This prevents the server from sending the final null chunk, alerting
// clients that the response was terminated prior to receiving full contents.
// Note that in the case of http/2 this does not close a connection, which
// would break other active requests, only resets the stream.
log.warn("Closing the connection to alert the client of an error");
IoUtils.safeClose(exchange.getConnection());
} catch (IOException | RuntimeException e) {
log.info("Failed to write error response", e);
}
} else {
// This prevents the server from sending the final null chunk, alerting
// clients that the response was terminated prior to receiving full contents.
// Note that in the case of http/2 this does not close a connection, which
// would break other active requests, only resets the stream.
log.warn("Closing the connection to alert the client of an error");
IoUtils.safeClose(exchange.getConnection());
}
}

Expand All @@ -154,4 +189,45 @@ private static void log(ServiceException exception) {
SafeArg.of("errorInstanceId", exception.getErrorInstanceId()), exception);
}
}

private static final QosException.Visitor<Integer> QOS_EXCEPTION_STATUS_CODE = new QosException.Visitor<Integer>() {
@Override
public Integer visit(QosException.Throttle exception) {
return 429;
}

@Override
public Integer visit(QosException.RetryOther exception) {
return 308;
}

@Override
public Integer visit(QosException.Unavailable exception) {
return 503;
}
};

private static final QosException.Visitor<Consumer<HttpServerExchange>>
QOS_EXCEPTION_HEADERS = new QosException.Visitor<Consumer<HttpServerExchange>>() {
@Override
public Consumer<HttpServerExchange> visit(QosException.Throttle exception) {
return exchange -> exception.getRetryAfter().ifPresent(duration -> {
exchange.getResponseHeaders()
.put(Headers.RETRY_AFTER, Long.toString(duration.get(ChronoUnit.SECONDS)));
});
}

@Override
public Consumer<HttpServerExchange> visit(QosException.RetryOther exception) {
return exchange -> exchange.getResponseHeaders()
.put(Headers.LOCATION, exception.getRedirectTo().toString());
}

@Override
public Consumer<HttpServerExchange> visit(QosException.Unavailable exception) {
return exchange -> {

};
}
};
}
50 changes: 49 additions & 1 deletion ...src/test/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@

import static org.assertj.core.api.Assertions.assertThat;

import com.google.common.collect.ImmutableList;
import com.palantir.conjure.java.api.errors.ErrorType;
import com.palantir.conjure.java.api.errors.QosException;
import com.palantir.conjure.java.api.errors.RemoteException;
import com.palantir.conjure.java.api.errors.SerializableError;
import com.palantir.conjure.java.api.errors.ServiceException;
Expand All @@ -27,6 +29,8 @@
import io.undertow.server.handlers.BlockingHandler;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.net.URL;
import java.time.Duration;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;
Expand All @@ -36,7 +40,9 @@

public final class ConjureExceptionHandlerTest {

private static final OkHttpClient client = new OkHttpClient.Builder().build();
private static final OkHttpClient client = new OkHttpClient.Builder()
.followRedirects(false) // we want to explicitly test the 'Location' header
.build();

private RuntimeException exception;
private Undertow server;
Expand Down Expand Up @@ -90,6 +96,48 @@ public void handlesRemoteException() throws IOException {
assertThat(response.code()).isEqualTo(ErrorType.INTERNAL.httpErrorCode());
}

@Test
public void handlesQosExceptionThrottleWithoutDuration() throws IOException {
exception = QosException.throttle();
Response response = execute();

assertThat(response.code()).isEqualTo(429);
assertThat(response.body().string()).isEmpty();
assertThat(response.headers().toMultimap()).doesNotContainKey("Retry-After");
assertThat(response.headers().toMultimap()).containsOnlyKeys("connection", "content-length", "date");
}

@Test
public void handlesQosExceptionThrottleWithDuration() throws IOException {
exception = QosException.throttle(Duration.ofMinutes(2));
Response response = execute();

assertThat(response.code()).isEqualTo(429);
assertThat(response.headers().toMultimap())
.containsEntry("Retry-After", ImmutableList.of("120"));
assertThat(response.body().string()).isEmpty();
}

@Test
public void handlesQosExceptionRetryOther() throws IOException {
exception = QosException.retryOther(new URL("http://foo"));
Response response = execute();

assertThat(response.code()).isEqualTo(308);
assertThat(response.headers().toMultimap())
.containsEntry("Location", ImmutableList.of("http://foo"));
assertThat(response.body().string()).isEmpty();
}

@Test
public void handlesQosExceptionUnavailable() throws IOException {
exception = QosException.unavailable();
Response response = execute();

assertThat(response.code()).isEqualTo(503);
assertThat(response.body().string()).isEmpty();
}

@Test
public void handlesIllegalArgumentException() throws IOException {
exception = new IllegalArgumentException("Foo");
Expand Down

0 comments on commit 84cf861

Please sign in to comment.