From 74bdb094c7205bf97d6b851b91a60f0856d2d945 Mon Sep 17 00:00:00 2001 From: Robert Fink Date: Mon, 29 Apr 2019 10:47:47 +0200 Subject: [PATCH] Add support for call timeouts (#70) --- .../AbstractSampleServiceClientTest.java | 27 +++++----- .../dialogue/example/SampleServiceClient.java | 49 +++++++++---------- .../dialogue/HttpSampleServiceClientTest.java | 14 +++--- .../OkHttpSampleServiceClientTest.java | 16 +++--- .../java/dialogue/serde/ConjureBodySerDe.java | 16 +++--- .../conjure/java/dialogue/serde/Encoding.java | 3 +- .../java/dialogue/serde/Encodings.java | 6 +-- .../dialogue/serde/ConjureBodySerDeTest.java | 3 +- .../java/dialogue/serde/EncodingsTest.java | 3 +- .../com/palantir/dialogue/Deserializer.java | 4 +- 10 files changed, 72 insertions(+), 69 deletions(-) diff --git a/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractSampleServiceClientTest.java b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractSampleServiceClientTest.java index e5b8202c1..667a0f9dc 100644 --- a/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractSampleServiceClientTest.java +++ b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractSampleServiceClientTest.java @@ -32,14 +32,15 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.nio.file.Paths; +import java.time.Duration; import java.time.OffsetDateTime; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okhttp3.mockwebserver.RecordedRequest; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -47,6 +48,9 @@ public abstract class AbstractSampleServiceClientTest { + abstract SampleService createBlockingClient(URL baseUrl, Duration timeout); + abstract AsyncSampleService createAsyncClient(URL baseUrl, Duration timeout); + private static final String PATH = "myPath"; private static final OffsetDateTime HEADER = OffsetDateTime.parse("2018-07-19T08:11:21+00:00"); private static final ImmutableList QUERY = ImmutableList.of( @@ -57,9 +61,6 @@ public abstract class AbstractSampleServiceClientTest { private static final SampleObject RESPONSE = new SampleObject(84); private static final String RESPONSE_STRING = "{\"intProperty\": 84}"; - abstract SampleService createBlockingClient(URL baseUrl); - abstract AsyncSampleService createAsyncClient(URL baseUrl); - static final SslConfiguration SSL_CONFIG = SslConfiguration.of( Paths.get("src/test/resources/trustStore.jks"), Paths.get("src/test/resources/keyStore.jks"), @@ -106,8 +107,8 @@ public abstract class AbstractSampleServiceClientTest { @Before public void before() { server.useHttps(SslSocketFactories.createSslSocketFactory(SSL_CONFIG), false); - blockingClient = createBlockingClient(server.url("").url()); - asyncClient = createAsyncClient(server.url("").url()); + blockingClient = createBlockingClient(server.url("").url(), Duration.ofSeconds(1)); + asyncClient = createAsyncClient(server.url("").url(), Duration.ofSeconds(1)); } @Test @@ -190,7 +191,7 @@ public void testAsync_voidToVoid_throwsWhenResponseBodyIsNonEmpty() { .hasMessageContaining("Expected empty response body"); } - @Test + @Test(timeout = 2_000) public void testBlocking_throwsOnConnectError() throws Exception { server.shutdown(); assertThatThrownBy(() -> blockingClient.objectToObject(PATH, HEADER, QUERY, BODY)) @@ -199,19 +200,21 @@ public void testBlocking_throwsOnConnectError() throws Exception { .hasMessageMatching(".*((Connection refused)|(Failed to connect)).*"); } - @Ignore("TODO(rfink): Figure out how to inject read/write timeouts") - @Test(timeout = 2_000) - public void testBlocking_throwsOnTimeout() { + @Test(timeout = 2_000) // see client construction: we set a 1s timeout + public void testBlocking_throwsOnTimeout() throws Exception { server.enqueue(new MockResponse() .setBody("\"response\"") .addHeader(Headers.CONTENT_TYPE, "application/json") .setBodyDelay(10, TimeUnit.SECONDS)); assertThatThrownBy(() -> blockingClient.objectToObject(PATH, HEADER, QUERY, BODY)) .isInstanceOf(RuntimeException.class) - .hasMessageContaining("Failed to deserialize response"); + .hasCauseInstanceOf(TimeoutException.class) + .hasMessageContaining("Waited 1000 milliseconds"); + + // TODO(rfink): It seems that the OkHttp version of this tests leaves the connection open after the timeout. } - @Test + @Test(timeout = 2_000) public void testAsync_throwsOnConnectError() throws Exception { server.shutdown(); assertThatThrownBy(() -> asyncClient.voidToVoid().get()) diff --git a/dialogue-example/src/main/java/com/palantir/dialogue/example/SampleServiceClient.java b/dialogue-example/src/main/java/com/palantir/dialogue/example/SampleServiceClient.java index ac35c2467..c4a986a6a 100644 --- a/dialogue-example/src/main/java/com/palantir/dialogue/example/SampleServiceClient.java +++ b/dialogue-example/src/main/java/com/palantir/dialogue/example/SampleServiceClient.java @@ -30,16 +30,16 @@ import com.palantir.dialogue.PathTemplate; import com.palantir.dialogue.PlainSerDe; import com.palantir.dialogue.Request; -import com.palantir.dialogue.Response; import com.palantir.dialogue.Serializer; import com.palantir.dialogue.TypeMarker; import com.palantir.dialogue.UrlBuilder; import com.palantir.logsafe.Preconditions; import com.palantir.ri.ResourceIdentifier; -import java.io.IOException; +import java.time.Duration; import java.time.OffsetDateTime; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; // Example of the implementation code conjure would generate for a simple SampleService. public final class SampleServiceClient { @@ -80,8 +80,13 @@ public HttpMethod httpMethod() { } }; - /** Returns a new blocking {@link SampleService} implementation whose calls are executed on the given channel. */ - public static SampleService blocking(Channel channel, ConjureRuntime runtime) { + /** + * Returns a new blocking {@link SampleService} implementation whose calls are executed on the given channel. + * The {@code callTimeout} parameter indicates the maximum end-to-end life time for the blocking methods in this + * service; an exception is thrown when this duration is exceeded. + */ + // TODO(rfink): Consider using a builder pattern to construct clients + public static SampleService blocking(Channel channel, ConjureRuntime runtime, Duration callTimeout) { return new SampleService() { private Serializer sampleObjectToSampleObjectSerializer = @@ -105,10 +110,14 @@ public SampleObject objectToObject( .build(); Call call = channel.createCall(STRING_TO_STRING, request); - ListenableFuture response = Calls.toFuture(call); + ListenableFuture response = Futures.transform( + Calls.toFuture(call), + r -> sampleObjectToSampleObjectDeserializer.deserialize(r), + MoreExecutors.directExecutor()); try { - // TODO(rfink): Figure out how to inject read/write timeouts - return sampleObjectToSampleObjectDeserializer.deserialize(response.get()); + return response.get(callTimeout.toMillis(), TimeUnit.MILLISECONDS); + // TODO(rfink): Think about exception handling, in particular in the case of retries. Should this + // actually throw a TimeoutException? } catch (Throwable t) { throw Exceptions.unwrapExecutionException(t); } @@ -119,9 +128,12 @@ public void voidToVoid() { Request request = Request.builder().build(); Call call = channel.createCall(VOID_TO_VOID, request); - ListenableFuture response = Calls.toFuture(call); + ListenableFuture deserializedResponse = Futures.transform( + Calls.toFuture(call), + r -> voidToVoidDeserializer.deserialize(r), + MoreExecutors.directExecutor()); try { - voidToVoidDeserializer.deserialize(response.get()); + deserializedResponse.get(callTimeout.toMillis(), TimeUnit.MILLISECONDS); } catch (Throwable t) { throw Exceptions.unwrapExecutionException(t); } @@ -133,6 +145,7 @@ public void voidToVoid() { * Returns a new asynchronous {@link AsyncSampleService} implementation whose calls are executed on the given * channel. */ + // TODO(rfink): Consider using a builder pattern to construct clients public static AsyncSampleService async(Channel channel, ConjureRuntime runtime) { return new AsyncSampleService() { @@ -162,14 +175,7 @@ public ListenableFuture stringToString( Call call = channel.createCall(STRING_TO_STRING, request); return Futures.transform( Calls.toFuture(call), - response -> { - try { - // TODO(rfink): The try/catch is a bit odd here. - return sampleObjectToSampleObjectDeserializer.deserialize(response); - } catch (IOException e) { - throw new RuntimeException("Failed to deserialize response", e); - } - }, + response -> sampleObjectToSampleObjectDeserializer.deserialize(response), MoreExecutors.directExecutor()); } @@ -180,14 +186,7 @@ public ListenableFuture voidToVoid() { Call call = channel.createCall(VOID_TO_VOID, request); return Futures.transform( Calls.toFuture(call), - response -> { - try { - voidToVoidDeserializer.deserialize(response); - return null; - } catch (IOException e) { - throw new RuntimeException("Failed to deserialize response", e); - } - }, + response -> voidToVoidDeserializer.deserialize(response), MoreExecutors.directExecutor()); } }; diff --git a/dialogue-java-client/src/test/java/com/palantir/dialogue/HttpSampleServiceClientTest.java b/dialogue-java-client/src/test/java/com/palantir/dialogue/HttpSampleServiceClientTest.java index d8c2e7ae3..8063c25ca 100644 --- a/dialogue-java-client/src/test/java/com/palantir/dialogue/HttpSampleServiceClientTest.java +++ b/dialogue-java-client/src/test/java/com/palantir/dialogue/HttpSampleServiceClientTest.java @@ -16,7 +16,6 @@ package com.palantir.dialogue; -import com.google.common.util.concurrent.MoreExecutors; import com.palantir.conjure.java.config.ssl.SslSocketFactories; import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; import com.palantir.conjure.java.dialogue.serde.DefaultErrorDecoder; @@ -26,6 +25,7 @@ import java.net.URL; import java.net.http.HttpClient; import java.time.Duration; +import java.util.concurrent.Executors; import javax.net.ssl.SSLParameters; import org.junit.runner.RunWith; import org.mockito.junit.MockitoJUnitRunner; @@ -36,14 +36,14 @@ public final class HttpSampleServiceClientTest extends AbstractSampleServiceClie private static final ConjureRuntime runtime = DefaultConjureRuntime.builder().build(); @Override - SampleService createBlockingClient(URL baseUrl) { - Channel channel = createChannel(baseUrl, Duration.ofSeconds(1)); - return SampleServiceClient.blocking(channel, runtime); + SampleService createBlockingClient(URL baseUrl, Duration timeout) { + Channel channel = createChannel(baseUrl, timeout); + return SampleServiceClient.blocking(channel, runtime, timeout); } @Override - AsyncSampleService createAsyncClient(URL baseUrl) { - Channel channel = createChannel(baseUrl, Duration.ofSeconds(1)); + AsyncSampleService createAsyncClient(URL baseUrl, Duration timeout) { + Channel channel = createChannel(baseUrl, timeout); return SampleServiceClient.async(channel, runtime); } @@ -57,7 +57,7 @@ private HttpChannel createChannel(URL url, Duration timeout) { .sslParameters(sslConfig) .sslContext(SslSocketFactories.createSslContext(SSL_CONFIG)) .build(), - MoreExecutors.newDirectExecutorService(), + Executors.newSingleThreadExecutor(), url, DefaultErrorDecoder.INSTANCE); } diff --git a/dialogue-okhttp-client/src/test/java/com/palantir/dialogue/OkHttpSampleServiceClientTest.java b/dialogue-okhttp-client/src/test/java/com/palantir/dialogue/OkHttpSampleServiceClientTest.java index 2a6437ff4..2587858ff 100644 --- a/dialogue-okhttp-client/src/test/java/com/palantir/dialogue/OkHttpSampleServiceClientTest.java +++ b/dialogue-okhttp-client/src/test/java/com/palantir/dialogue/OkHttpSampleServiceClientTest.java @@ -17,7 +17,6 @@ package com.palantir.dialogue; import com.google.common.collect.ImmutableList; -import com.google.common.util.concurrent.MoreExecutors; import com.palantir.conjure.java.config.ssl.SslSocketFactories; import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; import com.palantir.conjure.java.dialogue.serde.DefaultErrorDecoder; @@ -26,6 +25,7 @@ import com.palantir.dialogue.example.SampleServiceClient; import java.net.URL; import java.time.Duration; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import okhttp3.Dispatcher; import okhttp3.OkHttpClient; @@ -39,14 +39,14 @@ public final class OkHttpSampleServiceClientTest extends AbstractSampleServiceCl private static final ConjureRuntime runtime = DefaultConjureRuntime.builder().build(); @Override - SampleService createBlockingClient(URL baseUrl) { - Channel channel = createChannel(baseUrl, Duration.ofSeconds(1)); - return SampleServiceClient.blocking(channel, runtime); + SampleService createBlockingClient(URL baseUrl, Duration timeout) { + Channel channel = createChannel(baseUrl, timeout); + return SampleServiceClient.blocking(channel, runtime, timeout); } @Override - AsyncSampleService createAsyncClient(URL baseUrl) { - Channel channel = createChannel(baseUrl, Duration.ofSeconds(1)); + AsyncSampleService createAsyncClient(URL baseUrl, Duration timeout) { + Channel channel = createChannel(baseUrl, timeout); return SampleServiceClient.async(channel, runtime); } @@ -55,10 +55,8 @@ private OkHttpChannel createChannel(URL url, Duration timeout) { new OkHttpClient.Builder() .protocols(ImmutableList.of(Protocol.HTTP_1_1)) // Execute calls on same thread so that async tests are deterministic. - .dispatcher(new Dispatcher(MoreExecutors.newDirectExecutorService())) + .dispatcher(new Dispatcher(Executors.newSingleThreadExecutor())) .connectTimeout(timeout.toMillis(), TimeUnit.MILLISECONDS) - .readTimeout(timeout.toMillis(), TimeUnit.MILLISECONDS) - .writeTimeout(timeout.toMillis(), TimeUnit.MILLISECONDS) .sslSocketFactory( SslSocketFactories.createSslSocketFactory(SSL_CONFIG), SslSocketFactories.createX509TrustManager(SSL_CONFIG)) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java index dde27b0f2..ecdd4923d 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java @@ -28,7 +28,7 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; -import com.palantir.logsafe.exceptions.SafeIoException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -68,8 +68,12 @@ public Deserializer deserializer(TypeMarker token) { @Override public Deserializer emptyBodyDeserializer() { return response -> { - if (response.body().read() != -1) { - throw new RuntimeException("Expected empty response body"); + try { + if (response.body().read() != -1) { + throw new RuntimeException("Expected empty response body"); + } + } catch (IOException e) { + throw new RuntimeException("Failed to read from response body", e); } return null; }; @@ -171,7 +175,7 @@ private static final class EncodingDeserializerRegistry implements Deserializ } @Override - public T deserialize(Response response) throws IOException { + public T deserialize(Response response) { EncodingDeserializerContainer container = getResponseDeserializer(response.contentType()); return container.deserializer.deserialize(response.body()); } @@ -179,7 +183,7 @@ public T deserialize(Response response) throws IOException { /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */ @SuppressWarnings("ForLoopReplaceableByForEach") // performance sensitive code avoids iterator allocation - EncodingDeserializerContainer getResponseDeserializer(Optional contentType) throws SafeIoException { + EncodingDeserializerContainer getResponseDeserializer(Optional contentType) { if (!contentType.isPresent()) { throw new SafeIllegalArgumentException("Response is missing Content-Type header"); } @@ -189,7 +193,7 @@ EncodingDeserializerContainer getResponseDeserializer(Optional conten return container; } } - throw new SafeIoException("Unsupported Content-Type", SafeArg.of("Content-Type", contentType)); + throw new SafeRuntimeException("Unsupported Content-Type", SafeArg.of("Content-Type", contentType)); } } diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java index ec1d05150..f9fe8065e 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java @@ -17,7 +17,6 @@ package com.palantir.conjure.java.dialogue.serde; import com.palantir.dialogue.TypeMarker; -import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -62,7 +61,7 @@ interface Deserializer { * Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. */ - T deserialize(InputStream input) throws IOException; + T deserialize(InputStream input); } interface Serializer { diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encodings.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encodings.java index e8672f764..24ff176b2 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encodings.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encodings.java @@ -25,7 +25,7 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; -import com.palantir.logsafe.exceptions.SafeIoException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.FilterOutputStream; import java.io.IOException; import java.io.OutputStream; @@ -68,11 +68,11 @@ public Deserializer deserializer(TypeMarker type) { Preconditions.checkArgument(value != null, "cannot deserialize a JSON null value"); return value; } catch (MismatchedInputException e) { - throw new SafeIoException( + throw new SafeRuntimeException( "Failed to deserialize response stream. Syntax error?", e, SafeArg.of("type", type.getType())); } catch (IOException e) { - throw new SafeIoException( + throw new SafeRuntimeException( "Failed to deserialize response stream", e, SafeArg.of("type", type.getType())); } }; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java index 42db11c5b..dcfd0d794 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDeTest.java @@ -26,6 +26,7 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -63,7 +64,7 @@ public void testUnsupportedRequestContentType() { response.contentType = Optional.of("application/unknown"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(new StubEncoding("application/json"))); assertThatThrownBy(() -> serializers.deserializer(TYPE).deserialize(response)) - .isInstanceOf(IOException.class) + .isInstanceOf(SafeRuntimeException.class) .hasMessageContaining("Unsupported Content-Type"); } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EncodingsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EncodingsTest.java index 0496ab0de..55ae10aaa 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EncodingsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/EncodingsTest.java @@ -25,6 +25,7 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeNullPointerException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -43,7 +44,7 @@ public final class EncodingsTest { @Test public void json_deserialize_throwsDeserializationErrorsAsIllegalArgumentException() { assertThatThrownBy(() -> deserialize(asStream("\"2018-08-bogus\""), new TypeMarker() {})) - .isInstanceOf(IOException.class) + .isInstanceOf(SafeRuntimeException.class) .hasMessageContaining("Failed to deserialize"); } diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java b/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java index 34baffe45..51d6d1638 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java @@ -16,11 +16,9 @@ package com.palantir.dialogue; -import java.io.IOException; - /** Reads objects from a response. */ public interface Deserializer { /** Deserializes the response body. */ - T deserialize(Response response) throws IOException; + T deserialize(Response response); }