From 37d19450e57377200292018b7dbc5e2c741255d3 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Thu, 26 Mar 2020 23:40:58 +0000 Subject: [PATCH 01/38] BinaryReturnTypeTest --- dialogue-client-verifier/build.gradle | 5 +- .../verification/BinaryReturnTypeTest.java | 155 ++++++++++++++++++ dialogue-example/src/main/conjure/example.yml | 6 + versions.lock | 4 + versions.props | 2 + 5 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java diff --git a/dialogue-client-verifier/build.gradle b/dialogue-client-verifier/build.gradle index 80f04527e..2ec828f40 100644 --- a/dialogue-client-verifier/build.gradle +++ b/dialogue-client-verifier/build.gradle @@ -30,11 +30,14 @@ dependencies { testImplementation project('verification-server-api') testImplementation project(':dialogue-apache-hc4-client') testImplementation project(':dialogue-serde') + testImplementation project(':dialogue-example:dialogue-example-dialogue') testImplementation 'junit:junit' testImplementation 'org.assertj:assertj-core' testImplementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml' testImplementation 'org.apache.commons:commons-lang3' - testCompile 'com.palantir.conjure.java.runtime:keystores' + testImplementation 'com.palantir.conjure.java.runtime:keystores' + testImplementation 'io.undertow:undertow-core' + testImplementation 'com.squareup.okhttp3:mockwebserver' testRuntimeOnly 'org.apache.logging.log4j:log4j-slf4j-impl' testRuntimeOnly 'org.apache.logging.log4j:log4j-core' diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java new file mode 100644 index 000000000..0e3be80c5 --- /dev/null +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -0,0 +1,155 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.verification; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.collect.Iterables; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.palantir.conjure.java.api.config.service.ServiceConfiguration; +import com.palantir.conjure.java.api.config.service.UserAgent; +import com.palantir.conjure.java.api.config.service.UserAgents; +import com.palantir.conjure.java.api.config.ssl.SslConfiguration; +import com.palantir.conjure.java.client.config.ClientConfiguration; +import com.palantir.conjure.java.client.config.ClientConfigurations; +import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; +import com.palantir.dialogue.Channel; +import com.palantir.dialogue.example.SampleServiceAsync; +import com.palantir.dialogue.hc4.ApacheHttpClientChannels; +import io.undertow.Undertow; +import io.undertow.server.HttpHandler; +import io.undertow.server.HttpServerExchange; +import io.undertow.util.Headers; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; +import java.util.Optional; +import java.util.zip.GZIPOutputStream; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okio.Buffer; +import org.junit.Test; + +public class BinaryReturnTypeTest { + private static final UserAgent USER_AGENT = UserAgent.of(UserAgent.Agent.of("foo", "1.0.0")); + private static final SslConfiguration SSL_CONFIG = SslConfiguration.of( + Paths.get("../dialogue-core/src/test/resources/trustStore.jks"), + Paths.get("../dialogue-core/src/test" + "/resources/keyStore.jks"), + "keystore"); + + @Test + public void undertow_server() { + Undertow undertow = Undertow.builder() + .addHttpListener(0, "localhost", new HttpHandler() { + @Override + public void handleRequest(HttpServerExchange exchange) throws IOException { + exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); + exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); + exchange.dispatch(ex -> { + ex.startBlocking(); + ex.getOutputStream().write(gzipCompress("Hello, world")); + }); + } + }) + .build(); + try { + undertow.start(); + + String uri = getUri(undertow); + + SampleServiceAsync client = createClient(uri); + ListenableFuture> future = client.getOptionalBinary(); + + Optional maybeBinary = Futures.getUnchecked(future); + + assertThat(maybeBinary).isPresent(); + assertThat(maybeBinary.get()) + .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); + } finally { + undertow.stop(); + } + } + + /** I made two tests for the same thing because I wasn't 100% sure I'd set the server up right. */ + @Test + public void mockwebserver() throws IOException { + try (MockWebServer server = new MockWebServer()) { + + server.start(); + String uri = "http://localhost:" + server.getPort(); + + server.enqueue(new MockResponse() + .setResponseCode(200) + .setHeader("Content-Type", "application/octet-stream") + .setHeader("Content-Encoding", "gzip") + .setBody(okioBuffer(gzipCompress("Hello, world"))) + .removeHeader("Content-Length")); + + SampleServiceAsync client = createClient(uri); + ListenableFuture> future = client.getOptionalBinary(); + + Optional maybeBinary = Futures.getUnchecked(future); + + assertThat(maybeBinary).isPresent(); + assertThat(maybeBinary.get()) + .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); + } + } + + private static ClientConfiguration clientConf(String uri) { + return ClientConfiguration.builder() + .from(ClientConfigurations.of(ServiceConfiguration.builder() + .addUris(uri) + .security(SSL_CONFIG) + .build())) + .userAgent(USER_AGENT) + .build(); + } + + private static byte[] gzipCompress(String stringToCompress) throws IOException { + try (ByteArrayOutputStream baos = new ByteArrayOutputStream(); + GZIPOutputStream gzipOutput = new GZIPOutputStream(baos)) { + gzipOutput.write(stringToCompress.getBytes(StandardCharsets.UTF_8)); + gzipOutput.finish(); + return baos.toByteArray(); + } + } + + private static Buffer okioBuffer(byte[] byteArray) { + Buffer buffer = new Buffer(); + buffer.read(byteArray); + return buffer; + } + + private static SampleServiceAsync createClient(String address) { + Channel channel = ApacheHttpClientChannels.create(ClientConfiguration.builder() + .userAgent(UserAgents.parse("FooTest/0.0.0")) + .from(clientConf(address)) + .build()); + + return SampleServiceAsync.of(channel, DefaultConjureRuntime.builder().build()); + } + + private static String getUri(Undertow undertow) { + Undertow.ListenerInfo listenerInfo = Iterables.getOnlyElement(undertow.getListenerInfo()); + return String.format("%s:/%s", listenerInfo.getProtcol(), listenerInfo.getAddress()); + } +} diff --git a/dialogue-example/src/main/conjure/example.yml b/dialogue-example/src/main/conjure/example.yml index 04024d43d..0ac715f42 100644 --- a/dialogue-example/src/main/conjure/example.yml +++ b/dialogue-example/src/main/conjure/example.yml @@ -12,8 +12,10 @@ services: package: com.palantir.dialogue.example default-auth: none endpoints: + voidToVoid: http: GET /voidToVoid + objectToObject: http: POST /objectToObject/objects/{path} args: @@ -28,3 +30,7 @@ services: body: SampleObject returns: SampleObject + getOptionalBinary: + http: GET /getOptionalBinary + returns: optional + diff --git a/versions.lock b/versions.lock index 86311a032..ee27da156 100644 --- a/versions.lock +++ b/versions.lock @@ -59,6 +59,7 @@ com.spotify.dataenum:dataenum:1.3.2 (1 constraints: e91058c1) com.squareup.okhttp3:mockwebserver:3.13.1 (1 constraints: 3a053f3b) de.erichseifert.vectorgraphics2d:VectorGraphics2D:0.13 (1 constraints: 8c0a80bb) de.rototor.pdfbox:graphics2d:0.25 (1 constraints: 8f0a84bb) +io.undertow:undertow-core:2.0.30.Final (1 constraints: 4f070761) junit:junit:4.12 (2 constraints: e213d85a) net.bytebuddy:byte-buddy:1.10.5 (1 constraints: 410b37de) net.bytebuddy:byte-buddy-agent:1.10.5 (1 constraints: 410b37de) @@ -77,6 +78,9 @@ org.awaitility:awaitility:4.0.2 (1 constraints: 08050136) org.hamcrest:hamcrest:2.2 (3 constraints: f41da570) org.hamcrest:hamcrest-core:2.2 (3 constraints: 2f17d637) org.hamcrest:hamcrest-library:2.1 (1 constraints: 1507415c) +org.jboss.logging:jboss-logging:3.4.0.Final (1 constraints: bd0d7630) +org.jboss.xnio:xnio-api:3.3.8.Final (2 constraints: 6f1a4d45) +org.jboss.xnio:xnio-nio:3.3.8.Final (1 constraints: c40da530) org.jmock:jmock:2.12.0 (1 constraints: 3705353b) org.jmock:jmock-testjar:2.12.0 (1 constraints: a507a272) org.junit:junit-bom:5.6.1 (6 constraints: 406265f6) diff --git a/versions.props b/versions.props index 9eb36ad3a..ac45863cd 100644 --- a/versions.props +++ b/versions.props @@ -33,6 +33,8 @@ org.awaitility:awaitility = 4.0.2 org.jmock:jmock = 2.12.0 org.knowm.xchart:xchart = 3.6.1 com.palantir.conjure.verification:* = 0.18.3 +io.undertow:undertow-core = 2.0.30.Final +com.squareup.okhttp3:mockwebserver = 3.13.1 # dependency-upgrader:OFF # Match conjure-java-runtime okhttp version From 8d5565dc23a0920a96f8074e3cf42cec060d1798 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 00:40:52 +0000 Subject: [PATCH 02/38] Try turning off ContentDecodingChannel entirely --- .../com/palantir/dialogue/hc4/ApacheHttpClientChannels.java | 4 ++-- .../main/java/com/palantir/dialogue/core/DialogueChannel.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java b/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java index f5957b8f0..f08bcd298 100644 --- a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java +++ b/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java @@ -161,8 +161,8 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf .disableConnectionState() // Match okhttp behavior disabling cookies .disableCookieManagement() - // Dialogue handles content-compression with ContentDecodingChannel - .disableContentCompression() + // // Dialogue handles content-compression with ContentDecodingChannel + // .disableContentCompression() .setSSLSocketFactory(sslSocketFactory) .setDefaultCredentialsProvider(NullCredentialsProvider.INSTANCE) .setTargetAuthenticationStrategy(NullAuthenticationStrategy.INSTANCE) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java index 64b1a7e67..c973ce630 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java @@ -221,7 +221,7 @@ private static Channel wrap( channel = retryingChannel(channel, channelName, conf, scheduler, random); channel = new UserAgentChannel(channel, conf.userAgent().get()); channel = new DeprecationWarningChannel(channel, clientMetrics); - channel = new ContentDecodingChannel(channel); + // channel = new ContentDecodingChannel(channel); channel = new NeverThrowChannel(channel); channel = new TracedChannel(channel, "Dialogue-request"); channel = new ActiveRequestInstrumentationChannel(channel, channelName, "processing", clientMetrics); From d24f8ce7f2aa9f4cec6cc063ab452e2a3546221d Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 00:50:05 +0000 Subject: [PATCH 03/38] make the test green --- .../verification/BinaryReturnTypeTest.java | 27 +++++++++++++------ .../java/dialogue/serde/ConjureBodySerDe.java | 2 +- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index 0e3be80c5..8ad96ab28 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -46,6 +46,7 @@ import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.MockWebServer; import okio.Buffer; +import org.junit.Ignore; import org.junit.Test; public class BinaryReturnTypeTest { @@ -75,7 +76,8 @@ public void handleRequest(HttpServerExchange exchange) throws IOException { String uri = getUri(undertow); - SampleServiceAsync client = createClient(uri); + SampleServiceAsync client = SampleServiceAsync.of( + smartChannel(uri), DefaultConjureRuntime.builder().build()); ListenableFuture> future = client.getOptionalBinary(); Optional maybeBinary = Futures.getUnchecked(future); @@ -90,6 +92,7 @@ public void handleRequest(HttpServerExchange exchange) throws IOException { /** I made two tests for the same thing because I wasn't 100% sure I'd set the server up right. */ @Test + @Ignore // I don't know why this is hanging... public void mockwebserver() throws IOException { try (MockWebServer server = new MockWebServer()) { @@ -103,14 +106,24 @@ public void mockwebserver() throws IOException { .setBody(okioBuffer(gzipCompress("Hello, world"))) .removeHeader("Content-Length")); - SampleServiceAsync client = createClient(uri); + // ApacheHttpClientChannels.CloseableClient apache = + // ApacheHttpClientChannels.createCloseableHttpClient(clientConf(uri), "foo"); + // Channel dumbChannel = ApacheHttpClientChannels.createSingleUri(uri, apache); + // + // SampleServiceAsync dumbClient = SampleServiceAsync.of( + // dumbChannel, DefaultConjureRuntime.builder().build()); + SampleServiceAsync client = SampleServiceAsync.of( + smartChannel(uri), DefaultConjureRuntime.builder().build()); + ListenableFuture> future = client.getOptionalBinary(); Optional maybeBinary = Futures.getUnchecked(future); assertThat(maybeBinary).isPresent(); - assertThat(maybeBinary.get()) - .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); + try (InputStream actual = maybeBinary.get()) { + assertThat(actual) + .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); + } } } @@ -139,13 +152,11 @@ private static Buffer okioBuffer(byte[] byteArray) { return buffer; } - private static SampleServiceAsync createClient(String address) { - Channel channel = ApacheHttpClientChannels.create(ClientConfiguration.builder() + private static Channel smartChannel(String address) { + return ApacheHttpClientChannels.create(ClientConfiguration.builder() .userAgent(UserAgents.parse("FooTest/0.0.0")) .from(clientConf(address)) .build()); - - return SampleServiceAsync.of(channel, DefaultConjureRuntime.builder().build()); } private static String getUri(Undertow undertow) { 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 aa7345262..c9539c788 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 @@ -225,7 +225,7 @@ public T deserialize(Response response) { EncodingDeserializerContainer container = getResponseDeserializer(contentType); return container.deserializer.deserialize(response.body()); } finally { - response.close(); + // response.close(); } } From b7581358424455d70d723bd7885b7ddf361fa252 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:14:01 +0000 Subject: [PATCH 04/38] CloseableInputStream proves ConjureBodySerde is broken --- .../java/dialogue/serde/ConjureBodySerDe.java | 2 +- .../dialogue/serde/CloseableInputStream.java | 94 +++++++++++++++++++ .../dialogue/serde/ConjureBodySerDeTest.java | 18 +++- 3 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java 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 5a1b4b83a..06ab40717 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 @@ -230,7 +230,7 @@ public T deserialize(Response response) { EncodingDeserializerContainer container = getResponseDeserializer(contentType.get()); return container.deserializer.deserialize(response.body()); } finally { - // response.close(); + response.close(); } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java new file mode 100644 index 000000000..5cdd5d192 --- /dev/null +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java @@ -0,0 +1,94 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import com.google.common.annotations.VisibleForTesting; +import com.palantir.logsafe.exceptions.SafeIllegalStateException; +import java.io.IOException; +import java.io.InputStream; + +/** An inputstream which can only be closed once. */ +final class CloseableInputStream extends InputStream { + + private final InputStream delegate; + private boolean closed = false; + + CloseableInputStream(InputStream delegate) { + this.delegate = delegate; + } + + @VisibleForTesting + boolean isClosed() { + return closed; + } + + @Override + public int read() throws IOException { + checkPrecondition(); + return delegate.read(); + } + + @Override + public int read(byte[] bytes) throws IOException { + checkPrecondition(); + return delegate.read(bytes); + } + + @Override + public int read(byte[] bytes, int off, int len) throws IOException { + checkPrecondition(); + return delegate.read(bytes, off, len); + } + + @Override + public int available() throws IOException { + checkPrecondition(); + return delegate.available(); + } + + @Override + public void reset() throws IOException { + checkPrecondition(); + delegate.reset(); + } + + @Override + public synchronized void mark(int readlimit) { + if (closed) { + return; + } + delegate.mark(readlimit); + } + + @Override + public long skip(long num) throws IOException { + checkPrecondition(); + return delegate.skip(num); + } + + @Override + public void close() throws IOException { + closed = true; + delegate.close(); + } + + private void checkPrecondition() { + if (closed) { + throw new SafeIllegalStateException("Stream closed"); + } + } +} 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 f08e68ba1..f3be9be99 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 @@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.when; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; @@ -37,7 +38,6 @@ import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.IOException; -import java.io.InputStream; import java.util.Optional; import javax.ws.rs.core.HttpHeaders; import org.junit.Test; @@ -239,12 +239,13 @@ public String toString() { private static final class TestResponse implements Response { - private InputStream body = new ByteArrayInputStream(new byte[] {}); + private final CloseableInputStream body = + new CloseableInputStream(new ByteArrayInputStream(new byte[]{})); private int code = 0; private ListMultimap headers = ImmutableListMultimap.of(); @Override - public InputStream body() { + public CloseableInputStream body() { return body; } @@ -259,9 +260,16 @@ public ListMultimap headers() { } @Override - public void close() {} + public void close() { + try { + body.close(); + } catch (IOException e) { + throw new SafeRuntimeException("Failed to close", e); + } + } - public void contentType(String contentType) { + @VisibleForTesting + void contentType(String contentType) { this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); } } From 4ba85676ff6c75ec5225d965603c473a4cc93066 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:19:35 +0000 Subject: [PATCH 05/38] Show exactly where the close was called --- ...am.java => CloseRecordingInputStream.java} | 25 ++++++++----------- .../dialogue/serde/ConjureBodySerDeTest.java | 10 ++++---- 2 files changed, 16 insertions(+), 19 deletions(-) rename dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/{CloseableInputStream.java => CloseRecordingInputStream.java} (76%) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java similarity index 76% rename from dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java rename to dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java index 5cdd5d192..591eb234b 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseableInputStream.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java @@ -16,24 +16,23 @@ package com.palantir.conjure.java.dialogue.serde; -import com.google.common.annotations.VisibleForTesting; import com.palantir.logsafe.exceptions.SafeIllegalStateException; import java.io.IOException; import java.io.InputStream; +import java.util.Optional; -/** An inputstream which can only be closed once. */ -final class CloseableInputStream extends InputStream { +/** A test-only inputstream which can only be closed once. */ +final class CloseRecordingInputStream extends InputStream { private final InputStream delegate; - private boolean closed = false; + private Optional closeCalled = Optional.empty(); - CloseableInputStream(InputStream delegate) { + CloseRecordingInputStream(InputStream delegate) { this.delegate = delegate; } - @VisibleForTesting boolean isClosed() { - return closed; + return !closeCalled.isPresent(); } @Override @@ -67,10 +66,8 @@ public void reset() throws IOException { } @Override - public synchronized void mark(int readlimit) { - if (closed) { - return; - } + public void mark(int readlimit) { + checkPrecondition(); delegate.mark(readlimit); } @@ -82,13 +79,13 @@ public long skip(long num) throws IOException { @Override public void close() throws IOException { - closed = true; + closeCalled = Optional.of(new RuntimeException("close was called here")); delegate.close(); } private void checkPrecondition() { - if (closed) { - throw new SafeIllegalStateException("Stream closed"); + if (closeCalled.isPresent()) { + throw new SafeIllegalStateException("Can't do stuff after InputStream closed", closeCalled.get()); } } } 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 f3be9be99..3e3f944de 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 @@ -239,14 +239,14 @@ public String toString() { private static final class TestResponse implements Response { - private final CloseableInputStream body = - new CloseableInputStream(new ByteArrayInputStream(new byte[]{})); + private final CloseRecordingInputStream inputStream = + new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {})); private int code = 0; private ListMultimap headers = ImmutableListMultimap.of(); @Override - public CloseableInputStream body() { - return body; + public CloseRecordingInputStream body() { + return inputStream; } @Override @@ -262,7 +262,7 @@ public ListMultimap headers() { @Override public void close() { try { - body.close(); + inputStream.close(); } catch (IOException e) { throw new SafeRuntimeException("Failed to close", e); } From a10d2f00bf7fd6c438626a4fee439595f4357328 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:27:02 +0000 Subject: [PATCH 06/38] ConjureBodySerde should not be responsible for closing --- .../java/dialogue/serde/ConjureBodySerDe.java | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) 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 06ab40717..8301929b6 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 @@ -210,28 +210,26 @@ private static final class EncodingDeserializerRegistry implements Deserializ @Override public T deserialize(Response response) { - try { - if (errorDecoder.isError(response)) { - throw errorDecoder.decode(response); - } else if (response.code() == 204) { - if (!isOptionalType) { - throw new SafeRuntimeException( - "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); - } + if (errorDecoder.isError(response)) { + throw errorDecoder.decode(response); + } else if (response.code() == 204) { + if (!isOptionalType) { + throw new SafeRuntimeException( + "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); + } else { return TypeMarkers.getEmptyOptional(token); } + } - Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - if (!contentType.isPresent()) { - throw new SafeIllegalArgumentException( - "Response is missing Content-Type header", - SafeArg.of("received", response.headers().keySet())); - } - EncodingDeserializerContainer container = getResponseDeserializer(contentType.get()); - return container.deserializer.deserialize(response.body()); - } finally { + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + if (!contentType.isPresent()) { response.close(); + throw new SafeIllegalArgumentException( + "Response is missing Content-Type header", + SafeArg.of("received", response.headers().keySet())); } + EncodingDeserializerContainer container = getResponseDeserializer(contentType.get()); + return container.deserializer.deserialize(response.body()); } @Override From 37bffd17173c9a76653d37b751d5d759d596513c Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:40:24 +0000 Subject: [PATCH 07/38] Factor out TestResponse --- .../dialogue/serde/ConjureBodySerDeTest.java | 53 ++------------- .../java/dialogue/serde/TestResponse.java | 68 +++++++++++++++++++ 2 files changed, 73 insertions(+), 48 deletions(-) create mode 100644 dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java 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 3e3f944de..e5bd3a1ab 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 @@ -21,25 +21,19 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.when; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.ListMultimap; import com.palantir.conjure.java.api.errors.ErrorType; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.ServiceException; import com.palantir.dialogue.BodySerDe; import com.palantir.dialogue.RequestBody; -import com.palantir.dialogue.Response; 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.util.Optional; -import javax.ws.rs.core.HttpHeaders; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; @@ -70,7 +64,7 @@ public void testRequestContentType() throws IOException { @Test public void testRequestOptionalEmpty() { TestResponse response = new TestResponse(); - response.code = 204; + response.code(204); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); Optional value = serializers.deserializer(OPTIONAL_TYPE).deserialize(response); @@ -151,7 +145,7 @@ public void testResponseUnknownContentType() throws IOException { @Test public void testErrorsDecoded() { TestResponse response = new TestResponse(); - response.code = 400; + response.code(400); ServiceException serviceException = new ServiceException(ErrorType.INVALID_ARGUMENT); SerializableError serialized = SerializableError.forException(serviceException); @@ -168,7 +162,7 @@ public void testErrorsDecoded() { @Test public void testBinary() { TestResponse response = new TestResponse(); - response.code = 200; + response.code(200); response.contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); @@ -178,7 +172,7 @@ public void testBinary() { @Test public void testBinary_optional_present() { TestResponse response = new TestResponse(); - response.code = 200; + response.code(200); response.contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); @@ -189,7 +183,7 @@ public void testBinary_optional_present() { @Test public void testBinary_optional_empty() { TestResponse response = new TestResponse(); - response.code = 204; + response.code(204); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); assertThat(serializers.optionalInputStreamDeserializer().deserialize(response)) @@ -236,41 +230,4 @@ public String toString() { return "StubEncoding{" + contentType + '}'; } } - - private static final class TestResponse implements Response { - - private final CloseRecordingInputStream inputStream = - new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {})); - private int code = 0; - private ListMultimap headers = ImmutableListMultimap.of(); - - @Override - public CloseRecordingInputStream body() { - return inputStream; - } - - @Override - public int code() { - return code; - } - - @Override - public ListMultimap headers() { - return headers; - } - - @Override - public void close() { - try { - inputStream.close(); - } catch (IOException e) { - throw new SafeRuntimeException("Failed to close", e); - } - } - - @VisibleForTesting - void contentType(String contentType) { - this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); - } - } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java new file mode 100644 index 000000000..0a986eb46 --- /dev/null +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java @@ -0,0 +1,68 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ListMultimap; +import com.palantir.dialogue.Response; +import com.palantir.logsafe.exceptions.SafeRuntimeException; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import javax.ws.rs.core.HttpHeaders; + +final class TestResponse implements Response { + + private final CloseRecordingInputStream inputStream = + new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {})); + + private int code = 0; + private ListMultimap headers = ImmutableListMultimap.of(); + + @Override + public CloseRecordingInputStream body() { + return inputStream; + } + + @Override + public int code() { + return code; + } + + @Override + public ListMultimap headers() { + return headers; + } + + @Override + public void close() { + try { + inputStream.close(); + } catch (IOException e) { + throw new SafeRuntimeException("Failed to close", e); + } + } + + TestResponse code(int code) { + this.code = code; + return this; + } + + TestResponse contentType(String contentType) { + this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); + return this; + } +} From d249e5c41080807777b5771ff5234fbee4c79009 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:40:31 +0000 Subject: [PATCH 08/38] DEFAULT_ENCODINGS --- .../java/dialogue/serde/DefaultConjureRuntime.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java index afdc837fd..24b20bf78 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultConjureRuntime.java @@ -30,17 +30,17 @@ */ public final class DefaultConjureRuntime implements ConjureRuntime { + static final ImmutableList DEFAULT_ENCODINGS = ImmutableList.of( + WeightedEncoding.of(Encodings.json(), 1), + WeightedEncoding.of(Encodings.smile(), .9), + WeightedEncoding.of(Encodings.cbor(), .7)); + private final BodySerDe bodySerDe; private DefaultConjureRuntime(Builder builder) { this.bodySerDe = new ConjureBodySerDe( // TODO(rfink): The default thing here is a little odd - builder.encodings.isEmpty() - ? ImmutableList.of( - WeightedEncoding.of(Encodings.json(), 1), - WeightedEncoding.of(Encodings.smile(), .9), - WeightedEncoding.of(Encodings.cbor(), .7)) - : builder.encodings); + builder.encodings.isEmpty() ? DEFAULT_ENCODINGS : builder.encodings); } public static Builder builder() { From e772029f603dc61cdde9dec2b9e2d327d69a0cdd Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:54:12 +0000 Subject: [PATCH 09/38] better TestResponse and CloseRecordingInputStream --- .../dialogue/serde/CloseRecordingInputStream.java | 8 +++++++- .../conjure/java/dialogue/serde/TestResponse.java | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java index 591eb234b..c6c4a340f 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java @@ -32,7 +32,13 @@ final class CloseRecordingInputStream extends InputStream { } boolean isClosed() { - return !closeCalled.isPresent(); + return closeCalled.isPresent(); + } + + void assertUnClosed() { + if (isClosed()) { + throw new AssertionError("Expected CloseRecordingInputStream to be open but was closed", closeCalled.get()); + } } @Override diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java index 0a986eb46..e5fd855f3 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java @@ -22,6 +22,7 @@ import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.util.Optional; import javax.ws.rs.core.HttpHeaders; final class TestResponse implements Response { @@ -29,6 +30,7 @@ final class TestResponse implements Response { private final CloseRecordingInputStream inputStream = new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {})); + private Optional closeCalled = Optional.empty(); private int code = 0; private ListMultimap headers = ImmutableListMultimap.of(); @@ -49,13 +51,25 @@ public ListMultimap headers() { @Override public void close() { + checkPrecondition(); try { + closeCalled = Optional.of(new SafeRuntimeException("Close called here")); inputStream.close(); } catch (IOException e) { throw new SafeRuntimeException("Failed to close", e); } } + boolean isClosed() { + return closeCalled.isPresent(); + } + + private void checkPrecondition() { + if (closeCalled.isPresent()) { + throw new SafeRuntimeException("Please don't close twices", closeCalled.get()); + } + } + TestResponse code(int code) { this.code = code; return this; From 443aa538c058dbf1ffdd79a7e22d93cea442044f Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 01:56:42 +0000 Subject: [PATCH 10/38] test binary & optional responses --- .../dialogue/serde/DefaultClientsTest.java | 66 +++++++++++++++++-- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 6d8878730..413d231e4 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -28,12 +28,15 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; +import com.palantir.dialogue.BodySerDe; import com.palantir.dialogue.Channel; import com.palantir.dialogue.Deserializer; import com.palantir.dialogue.Endpoint; import com.palantir.dialogue.Request; import com.palantir.dialogue.RequestBody; import com.palantir.dialogue.Response; +import java.io.IOException; +import java.io.InputStream; import java.util.Optional; import java.util.concurrent.ExecutionException; import org.junit.Test; @@ -52,20 +55,20 @@ public final class DefaultClientsTest { @Mock private Endpoint endpoint; - @Mock - private Response response; - @Mock private Deserializer deserializer; @Captor private ArgumentCaptor requestCaptor; + private Response response = new TestResponse(); + private BodySerDe bodySerde = new ConjureBodySerDe(DefaultConjureRuntime.DEFAULT_ENCODINGS); + private final SettableFuture responseFuture = SettableFuture.create(); + @Test public void testCall() throws ExecutionException, InterruptedException { Request request = Request.builder().build(); when(deserializer.deserialize(eq(response))).thenReturn("value"); - SettableFuture responseFuture = SettableFuture.create(); when(channel.execute(eq(endpoint), eq(request))).thenReturn(responseFuture); ListenableFuture result = DefaultClients.INSTANCE.call(channel, endpoint, request, deserializer); assertThat(result).isNotDone(); @@ -97,7 +100,6 @@ public void testCallClosesRequestOnCompletion_success() { RequestBody body = mock(RequestBody.class); Request request = Request.builder().body(body).build(); when(deserializer.deserialize(eq(response))).thenReturn("value"); - SettableFuture responseFuture = SettableFuture.create(); when(channel.execute(eq(endpoint), eq(request))).thenReturn(responseFuture); ListenableFuture result = DefaultClients.INSTANCE.call(channel, endpoint, request, deserializer); @@ -110,11 +112,63 @@ public void testCallClosesRequestOnCompletion_success() { verify(body).close(); } + @Test + public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { + when(channel.execute(eq(endpoint), any())).thenReturn(responseFuture); + + ListenableFuture future = DefaultClients.INSTANCE.call( + channel, endpoint, Request.builder().build(), bodySerde.inputStreamDeserializer()); + + TestResponse testResponse = new TestResponse().contentType("application/octet-stream"); + responseFuture.set(testResponse); + + try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) Futures.getUnchecked(future)) { + assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); + inputStream.assertUnClosed(); + assertThat(testResponse.isClosed()) + .describedAs("TODO(dfox): what do we do with the actual response at this point??") + .isFalse(); + } + + assertThat(testResponse.body().isClosed()) + .describedAs("User has closed it now") + .isTrue(); + assertThat(testResponse.isClosed()) + .describedAs("TODO(dfox): I think this should magically be closed by now") + .isFalse(); + } + + @Test + public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOException { + when(channel.execute(eq(endpoint), any())).thenReturn(responseFuture); + + ListenableFuture> future = DefaultClients.INSTANCE.call( + channel, endpoint, Request.builder().build(), bodySerde.optionalInputStreamDeserializer()); + + TestResponse testResponse = new TestResponse().contentType("application/octet-stream"); + responseFuture.set(testResponse); + + Optional maybeInputStream = Futures.getUnchecked(future); + try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) maybeInputStream.get()) { + assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); + inputStream.assertUnClosed(); + assertThat(testResponse.isClosed()) + .describedAs("TODO(dfox): what do we do with the actual response at this point??") + .isFalse(); + } + + assertThat(testResponse.body().isClosed()) + .describedAs("User has closed it now") + .isTrue(); + assertThat(testResponse.isClosed()) + .describedAs("TODO(dfox): I think this should magically be closed by now") + .isFalse(); + } + @Test public void testCallClosesRequestOnCompletion_failure() { RequestBody body = mock(RequestBody.class); Request request = Request.builder().body(body).build(); - SettableFuture responseFuture = SettableFuture.create(); when(channel.execute(eq(endpoint), eq(request))).thenReturn(responseFuture); ListenableFuture result = DefaultClients.INSTANCE.call(channel, endpoint, request, deserializer); From a11d0f96fc994201aa9ff7d5252cb7fd37c21dde Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:00:49 +0000 Subject: [PATCH 11/38] Renames --- .../serde/CloseRecordingInputStream.java | 31 ++++++++----------- .../dialogue/serde/DefaultClientsTest.java | 4 +-- .../java/dialogue/serde/TestResponse.java | 6 ++-- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java index c6c4a340f..6f7e3a921 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java @@ -16,10 +16,11 @@ package com.palantir.conjure.java.dialogue.serde; -import com.palantir.logsafe.exceptions.SafeIllegalStateException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; import java.io.InputStream; import java.util.Optional; +import org.assertj.core.api.Assertions; /** A test-only inputstream which can only be closed once. */ final class CloseRecordingInputStream extends InputStream { @@ -35,63 +36,57 @@ boolean isClosed() { return closeCalled.isPresent(); } - void assertUnClosed() { - if (isClosed()) { - throw new AssertionError("Expected CloseRecordingInputStream to be open but was closed", closeCalled.get()); + void assertNotClosed() { + if (closeCalled.isPresent()) { + Assertions.fail("Expected CloseRecordingInputStream to be open but was closed", closeCalled.get()); } } @Override public int read() throws IOException { - checkPrecondition(); + assertNotClosed(); return delegate.read(); } @Override public int read(byte[] bytes) throws IOException { - checkPrecondition(); + assertNotClosed(); return delegate.read(bytes); } @Override public int read(byte[] bytes, int off, int len) throws IOException { - checkPrecondition(); + assertNotClosed(); return delegate.read(bytes, off, len); } @Override public int available() throws IOException { - checkPrecondition(); + assertNotClosed(); return delegate.available(); } @Override public void reset() throws IOException { - checkPrecondition(); + assertNotClosed(); delegate.reset(); } @Override public void mark(int readlimit) { - checkPrecondition(); + assertNotClosed(); delegate.mark(readlimit); } @Override public long skip(long num) throws IOException { - checkPrecondition(); + assertNotClosed(); return delegate.skip(num); } @Override public void close() throws IOException { - closeCalled = Optional.of(new RuntimeException("close was called here")); + closeCalled = Optional.of(new SafeRuntimeException("close was called here")); delegate.close(); } - - private void checkPrecondition() { - if (closeCalled.isPresent()) { - throw new SafeIllegalStateException("Can't do stuff after InputStream closed", closeCalled.get()); - } - } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 413d231e4..215570e54 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -124,7 +124,7 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) Futures.getUnchecked(future)) { assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); - inputStream.assertUnClosed(); + inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) .describedAs("TODO(dfox): what do we do with the actual response at this point??") .isFalse(); @@ -151,7 +151,7 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc Optional maybeInputStream = Futures.getUnchecked(future); try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) maybeInputStream.get()) { assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); - inputStream.assertUnClosed(); + inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) .describedAs("TODO(dfox): what do we do with the actual response at this point??") .isFalse(); diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java index e5fd855f3..a114076a2 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java @@ -51,7 +51,7 @@ public ListMultimap headers() { @Override public void close() { - checkPrecondition(); + checkNotClosed(); try { closeCalled = Optional.of(new SafeRuntimeException("Close called here")); inputStream.close(); @@ -64,9 +64,9 @@ boolean isClosed() { return closeCalled.isPresent(); } - private void checkPrecondition() { + private void checkNotClosed() { if (closeCalled.isPresent()) { - throw new SafeRuntimeException("Please don't close twices", closeCalled.get()); + throw new SafeRuntimeException("Please don't close twice", closeCalled.get()); } } From f24f5d30a793ddca0c9a8576a07c18382a97ef44 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:18:21 +0000 Subject: [PATCH 12/38] Minor fix --- .../conjure/java/dialogue/serde/BinaryEncoding.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java index ad66f5e44..f3bb20936 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java @@ -67,11 +67,12 @@ public String toString() { return "BinaryEncoding{" + CONTENT_TYPE + '}'; } - enum OptionalInputStreamDeserializer implements Deserializer { + enum OptionalInputStreamDeserializer implements Deserializer> { INSTANCE; @Override - public Object deserialize(InputStream input) { + public Optional deserialize(InputStream input) { + // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! return Optional.of(input); } @@ -81,11 +82,12 @@ public String toString() { } } - enum InputStreamDeserializer implements Deserializer { + enum InputStreamDeserializer implements Deserializer { INSTANCE; @Override - public Object deserialize(InputStream input) { + public InputStream deserialize(InputStream input) { + // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! return input; } From 9ee901f01a2b14692e94faecbfd36bbcb5e2347b Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:18:39 +0000 Subject: [PATCH 13/38] ErrorDecoder closes response & inputstream --- .../java/dialogue/serde/ErrorDecoder.java | 47 +++++++++++++------ .../java/dialogue/serde/ErrorDecoderTest.java | 12 +++++ 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java index f55fbd1da..22880e4eb 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java @@ -30,6 +30,8 @@ import java.nio.charset.StandardCharsets; import java.util.Optional; import javax.ws.rs.core.HttpHeaders; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Extracts and returns a {@link RemoteException} from an {@link Response}. @@ -40,6 +42,7 @@ enum ErrorDecoder { INSTANCE; + private static final Logger log = LoggerFactory.getLogger(ErrorDecoder.class); private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); boolean isError(Response response) { @@ -50,26 +53,40 @@ RemoteException decode(Response response) { // TODO(rfink): What about HTTP/101 switching protocols? // TODO(rfink): What about HEAD requests? - String body; + InputStream inputStream = response.body(); try { - body = toString(response.body()); - } catch (NullPointerException | IOException e) { - UnknownRemoteException exception = new UnknownRemoteException(response.code(), ""); - exception.initCause(e); - throw exception; - } + String body; + try { + body = toString(inputStream); + } catch (NullPointerException | IOException e) { + UnknownRemoteException exception = new UnknownRemoteException(response.code(), ""); + exception.initCause(e); + throw exception; + } + + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { + try { + SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); + return new RemoteException(serializableError, response.code()); + } catch (Exception e) { + throw new UnknownRemoteException(response.code(), body); + } + } - Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { + throw new UnknownRemoteException(response.code(), body); + } finally { + try { + inputStream.close(); + } catch (RuntimeException | IOException e) { + log.warn("Failed to close InputStream", e); + } try { - SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); - return new RemoteException(serializableError, response.code()); - } catch (Exception e) { - throw new UnknownRemoteException(response.code(), body); + response.close(); + } catch (RuntimeException e) { + log.warn("Failed to close Response", e); } } - - throw new UnknownRemoteException(response.code(), body); } private static String toString(InputStream body) throws IOException { diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index dcbdd6bd7..39b303889 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -146,6 +146,18 @@ public void handlesJsonWithEncoding() { assertThat(exception.getError().errorName()).isEqualTo(ErrorType.FAILED_PRECONDITION.name()); } + @Test + public void closes_response_and_inputstream() { + TestResponse testResponse = new TestResponse().contentType("application/json"); + assertThatThrownBy(() -> decoder.decode(testResponse)).isInstanceOf(UnknownRemoteException.class); + assertThat(testResponse.body().isClosed()) + .describedAs("Expected inputstream to be closed") + .isTrue(); + assertThat(testResponse.isClosed()) + .describedAs("Body should probably be closed too") + .isTrue(); + } + private static RemoteException encodeAndDecode(Exception exception) { Preconditions.checkArgument(!(exception instanceof ServiceException), "Use SerializableError#forException"); Object error = SerializableError.builder() From 8ac1c0bd959947f56894d2ef53347ffb6685e1ec Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:18:46 +0000 Subject: [PATCH 14/38] Update javadoc --- .../com/palantir/conjure/java/dialogue/serde/Encoding.java | 6 ++++-- .../src/main/java/com/palantir/dialogue/Deserializer.java | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) 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 3ac9e0b4e..b4e4fc695 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 @@ -57,8 +57,10 @@ interface Deserializer { /** * Reads a serialized type-{@link T} object representation from the given input stream and returns the - * corresponding object. Implementations should read the entire input stream, but must not close it. - * Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs + * corresponding object. Implementations should read the entire input stream and must close it (unless they + * return the raw InputStream, e.g. for a binary response type). + * + *

Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. */ T deserialize(InputStream input); 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 fcf5faddb..51be50efe 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java @@ -21,7 +21,10 @@ /** Reads objects from a response. */ public interface Deserializer { - /** Deserializes the response body. */ + /** + * Deserializes the response body into a useful type. Closes the {@link Response#body()} inputStream unless + * the return type requires an {@code InputStream} to be returned in a readable state. + */ T deserialize(Response response); /** From 79ac9d1421ef42e0a527c398345a7c1e0aa04b87 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:18:57 +0000 Subject: [PATCH 15/38] format & checkstyle --- .../java/dialogue/serde/DefaultClientsTest.java | 8 ++++++-- .../conjure/java/dialogue/serde/TestResponse.java | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 215570e54..30070cd5f 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -123,7 +123,9 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { responseFuture.set(testResponse); try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) Futures.getUnchecked(future)) { - assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); + assertThat(inputStream.available()) + .describedAs("Content should be empty") + .isEqualTo(0); inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) .describedAs("TODO(dfox): what do we do with the actual response at this point??") @@ -150,7 +152,9 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc Optional maybeInputStream = Futures.getUnchecked(future); try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) maybeInputStream.get()) { - assertThat(inputStream.available()).describedAs("Content should be empty").isEqualTo(0); + assertThat(inputStream.available()) + .describedAs("Content should be empty") + .isEqualTo(0); inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) .describedAs("TODO(dfox): what do we do with the actual response at this point??") diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java index a114076a2..eb4790e4c 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java @@ -44,6 +44,11 @@ public int code() { return code; } + TestResponse code(int value) { + this.code = value; + return this; + } + @Override public ListMultimap headers() { return headers; @@ -70,11 +75,6 @@ private void checkNotClosed() { } } - TestResponse code(int code) { - this.code = code; - return this; - } - TestResponse contentType(String contentType) { this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); return this; From 4c9865265cc6b4f64f8ae081e4924974dd8cfc2f Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:44:33 +0000 Subject: [PATCH 16/38] Close out unknowns too --- .../java/dialogue/serde/ConjureBodySerDe.java | 48 ++++++++++++----- .../java/dialogue/serde/Encodings.java | 5 +- .../dialogue/serde/ConjureBodySerDeTest.java | 54 ++++++++++--------- .../java/dialogue/serde/TestResponse.java | 3 ++ 4 files changed, 72 insertions(+), 38 deletions(-) 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 8301929b6..7bfda2aa7 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 @@ -198,7 +198,7 @@ private static final class EncodingDeserializerRegistry implements Deserializ EncodingDeserializerRegistry(List encodings, ErrorDecoder errorDecoder, TypeMarker token) { this.encodings = encodings.stream() - .map(encoding -> new EncodingDeserializerContainer<>(encoding, token)) + .map(encoding -> EncodingDeserializerContainer.of(encoding, token)) .collect(ImmutableList.toImmutableList()); this.errorDecoder = errorDecoder; this.token = token; @@ -211,8 +211,13 @@ private static final class EncodingDeserializerRegistry implements Deserializ @Override public T deserialize(Response response) { if (errorDecoder.isError(response)) { - throw errorDecoder.decode(response); + try { + throw errorDecoder.decode(response); + } finally { + response.close(); + } } else if (response.code() == 204) { + response.close(); if (!isOptionalType) { throw new SafeRuntimeException( "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); @@ -228,8 +233,8 @@ public T deserialize(Response response) { "Response is missing Content-Type header", SafeArg.of("received", response.headers().keySet())); } - EncodingDeserializerContainer container = getResponseDeserializer(contentType.get()); - return container.deserializer.deserialize(response.body()); + Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); + return deserializer.deserialize(response.body()); } @Override @@ -240,28 +245,47 @@ public Optional accepts() { /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */ @SuppressWarnings("ForLoopReplaceableByForEach") // performance sensitive code avoids iterator allocation - EncodingDeserializerContainer getResponseDeserializer(String contentType) { + Encoding.Deserializer getResponseDeserializer(String contentType) { for (int i = 0; i < encodings.size(); i++) { EncodingDeserializerContainer container = encodings.get(i); if (container.encoding.supportsContentType(contentType)) { - return container; + return container.deserializer; } } - throw new SafeRuntimeException( - "Unsupported Content-Type", - SafeArg.of("received", contentType), - SafeArg.of("supportedEncodings", encodings)); + return throwingDeserializer(contentType); + } + + private Encoding.Deserializer throwingDeserializer(String contentType) { + return new Encoding.Deserializer() { + @Override + public T deserialize(InputStream input) { + try { + input.close(); + } catch (RuntimeException | IOException e) { + // empty + } + throw new SafeRuntimeException( + "Unsupported Content-Type", + SafeArg.of("received", contentType), + SafeArg.of("supportedEncodings", encodings)); + } + }; } } + /** Effectively just a pair. */ private static final class EncodingDeserializerContainer { private final Encoding encoding; private final Encoding.Deserializer deserializer; - EncodingDeserializerContainer(Encoding encoding, TypeMarker token) { + EncodingDeserializerContainer(Encoding encoding, Encoding.Deserializer deserializer) { this.encoding = encoding; - this.deserializer = encoding.deserializer(token); + this.deserializer = deserializer; + } + + static EncodingDeserializerContainer of(Encoding encoding, TypeMarker token) { + return new EncodingDeserializerContainer<>(encoding, encoding.deserializer(token)); } @Override 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 dbe764c6c..bd0816c42 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 @@ -27,6 +27,7 @@ import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; +import java.io.InputStream; import javax.annotation.Nullable; // TODO(rfink): Consider async Jackson, see @@ -61,8 +62,8 @@ public final Serializer serializer(TypeMarker type) { public final Deserializer deserializer(TypeMarker type) { ObjectReader reader = mapper.readerFor(mapper.constructType(type.getType())); return input -> { - try { - T value = reader.readValue(input); + try (InputStream inputStream = input) { + T value = reader.readValue(inputStream); // Bad input should result in a 4XX response status, throw IAE rather than NPE. Preconditions.checkArgument(value != null, "cannot deserialize a JSON null value"); return value; 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 e5bd3a1ab..1cdb8d78c 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 @@ -33,6 +33,7 @@ import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; +import java.io.InputStream; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -53,8 +54,7 @@ public void testRequestContentType() throws IOException { Encoding json = new StubEncoding("application/json"); Encoding plain = new StubEncoding("text/plain"); - TestResponse response = new TestResponse(); - response.contentType("text/plain"); + TestResponse response = new TestResponse().contentType("text/plain"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(json), WeightedEncoding.of(plain))); String value = serializers.deserializer(TYPE).deserialize(response); @@ -63,8 +63,7 @@ public void testRequestContentType() throws IOException { @Test public void testRequestOptionalEmpty() { - TestResponse response = new TestResponse(); - response.code(204); + TestResponse response = new TestResponse().code(204); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); Optional value = serializers.deserializer(OPTIONAL_TYPE).deserialize(response); @@ -83,8 +82,7 @@ public void testRequestNoContentType() { @Test public void testUnsupportedRequestContentType() { - TestResponse response = new TestResponse(); - response.contentType("application/unknown"); + TestResponse response = new TestResponse().contentType("application/unknown"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); assertThatThrownBy(() -> serializers.deserializer(TYPE).deserialize(response)) @@ -130,12 +128,10 @@ public void testResponseNoContentType() throws IOException { } @Test - public void testResponseUnknownContentType() throws IOException { + public void testRequestUnknownContentType() throws IOException { Encoding json = new StubEncoding("application/json"); Encoding plain = new StubEncoding("text/plain"); - TestResponse response = new TestResponse(); - response.contentType("application/unknown"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(json), WeightedEncoding.of(plain))); RequestBody body = serializers.serializer(TYPE).serialize("test"); @@ -144,8 +140,7 @@ public void testResponseUnknownContentType() throws IOException { @Test public void testErrorsDecoded() { - TestResponse response = new TestResponse(); - response.code(400); + TestResponse response = new TestResponse().code(400); ServiceException serviceException = new ServiceException(ErrorType.INVALID_ARGUMENT); SerializableError serialized = SerializableError.forException(serviceException); @@ -157,37 +152,48 @@ public void testErrorsDecoded() { assertThatExceptionOfType(RemoteException.class) .isThrownBy(() -> serializers.deserializer(TYPE).deserialize(response)); + + assertThat(response.isClosed()).describedAs("response should be closed").isTrue(); + assertThat(response.body().isClosed()) + .describedAs("inputstream should be closed") + .isTrue(); } @Test - public void testBinary() { - TestResponse response = new TestResponse(); - response.code(200); - response.contentType("application/octet-stream"); + public void testBinary() throws IOException { + TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); - assertThat(serializers.inputStreamDeserializer().deserialize(response)).hasContent(""); + assertThat(serializers.inputStreamDeserializer().deserialize(response).available()) + .isEqualTo(0); + response.body().assertNotClosed(); + assertThat(response.isClosed()).describedAs("TODO ???").isFalse(); } @Test - public void testBinary_optional_present() { - TestResponse response = new TestResponse(); - response.code(200); - response.contentType("application/octet-stream"); + public void testBinary_optional_present() throws IOException { + TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); - assertThat(serializers.optionalInputStreamDeserializer().deserialize(response)) - .hasValueSatisfying(stream -> assertThat(stream).hasContent("")); + Optional maybe = + serializers.optionalInputStreamDeserializer().deserialize(response); + assertThat(maybe).isPresent(); + assertThat(maybe.get().available()).isEqualTo(0); + response.body().assertNotClosed(); + assertThat(response.isClosed()).describedAs("TODO ???").isFalse(); } @Test public void testBinary_optional_empty() { - TestResponse response = new TestResponse(); - response.code(204); + TestResponse response = new TestResponse().code(204); BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); assertThat(serializers.optionalInputStreamDeserializer().deserialize(response)) .isEmpty(); + assertThat(response.body().isClosed()) + .describedAs("inputstream should be closed") + .isTrue(); + assertThat(response.isClosed()).describedAs("response should be closed").isTrue(); } /** Deserializes requests as the configured content type. */ diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java index eb4790e4c..c1419728b 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; +import com.google.errorprone.annotations.CheckReturnValue; import com.palantir.dialogue.Response; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; @@ -44,6 +45,7 @@ public int code() { return code; } + @CheckReturnValue TestResponse code(int value) { this.code = value; return this; @@ -75,6 +77,7 @@ private void checkNotClosed() { } } + @CheckReturnValue TestResponse contentType(String contentType) { this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); return this; From 1f43c4d4620e05f651c1975f531524c6a9b1625e Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 02:55:55 +0000 Subject: [PATCH 17/38] Extra safety net, just in case --- .../java/dialogue/serde/DefaultClients.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java index c60b85cf8..83bb8e9bc 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java @@ -57,7 +57,19 @@ public ListenableFuture call( Request outgoingRequest = accepts.isPresent() ? accepting(request, accepts.get()) : request; ListenableFuture response = closeRequestBodyOnCompletion(channel.execute(endpoint, outgoingRequest), outgoingRequest); - return Futures.transform(response, deserializer::deserialize, MoreExecutors.directExecutor()); + return Futures.transform( + response, + resp -> { + try { + return deserializer.deserialize(resp); + } catch (RuntimeException e) { + if (resp != null) { + resp.close(); // extra safety net, just in case something unexpected throws + } + throw e; + } + }, + MoreExecutors.directExecutor()); } private static ListenableFuture closeRequestBodyOnCompletion( From 4a5c246090baf695ad06b0b57b0e6191b9bb0b4c Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:00:14 +0000 Subject: [PATCH 18/38] Move useful types to :test-lib --- .../palantir/dialogue}/CloseRecordingInputStream.java | 10 +++++----- .../java/com/palantir/dialogue}/TestResponse.java | 11 +++++------ dialogue-serde/build.gradle | 1 + .../java/dialogue/serde/ConjureBodySerDeTest.java | 1 + .../java/dialogue/serde/DefaultClientsTest.java | 2 ++ .../conjure/java/dialogue/serde/ErrorDecoderTest.java | 1 + 6 files changed, 15 insertions(+), 11 deletions(-) rename {dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde => dialogue-client-test-lib/src/main/java/com/palantir/dialogue}/CloseRecordingInputStream.java (91%) rename {dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde => dialogue-client-test-lib/src/main/java/com/palantir/dialogue}/TestResponse.java (90%) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/CloseRecordingInputStream.java similarity index 91% rename from dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java rename to dialogue-client-test-lib/src/main/java/com/palantir/dialogue/CloseRecordingInputStream.java index 6f7e3a921..7bb896380 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/CloseRecordingInputStream.java +++ b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/CloseRecordingInputStream.java @@ -14,7 +14,7 @@ * limitations under the License. */ -package com.palantir.conjure.java.dialogue.serde; +package com.palantir.dialogue; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; @@ -23,20 +23,20 @@ import org.assertj.core.api.Assertions; /** A test-only inputstream which can only be closed once. */ -final class CloseRecordingInputStream extends InputStream { +public final class CloseRecordingInputStream extends InputStream { private final InputStream delegate; private Optional closeCalled = Optional.empty(); - CloseRecordingInputStream(InputStream delegate) { + public CloseRecordingInputStream(InputStream delegate) { this.delegate = delegate; } - boolean isClosed() { + public boolean isClosed() { return closeCalled.isPresent(); } - void assertNotClosed() { + public void assertNotClosed() { if (closeCalled.isPresent()) { Assertions.fail("Expected CloseRecordingInputStream to be open but was closed", closeCalled.get()); } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/TestResponse.java similarity index 90% rename from dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java rename to dialogue-client-test-lib/src/main/java/com/palantir/dialogue/TestResponse.java index c1419728b..a1bf03677 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/TestResponse.java +++ b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/TestResponse.java @@ -14,19 +14,18 @@ * limitations under the License. */ -package com.palantir.conjure.java.dialogue.serde; +package com.palantir.dialogue; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; import com.google.errorprone.annotations.CheckReturnValue; -import com.palantir.dialogue.Response; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; import java.io.IOException; import java.util.Optional; import javax.ws.rs.core.HttpHeaders; -final class TestResponse implements Response { +public final class TestResponse implements Response { private final CloseRecordingInputStream inputStream = new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {})); @@ -46,7 +45,7 @@ public int code() { } @CheckReturnValue - TestResponse code(int value) { + public TestResponse code(int value) { this.code = value; return this; } @@ -67,7 +66,7 @@ public void close() { } } - boolean isClosed() { + public boolean isClosed() { return closeCalled.isPresent(); } @@ -78,7 +77,7 @@ private void checkNotClosed() { } @CheckReturnValue - TestResponse contentType(String contentType) { + public TestResponse contentType(String contentType) { this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType); return this; } diff --git a/dialogue-serde/build.gradle b/dialogue-serde/build.gradle index 0a9a153e0..1b8aca44e 100644 --- a/dialogue-serde/build.gradle +++ b/dialogue-serde/build.gradle @@ -8,6 +8,7 @@ dependencies { compile 'com.palantir.safe-logging:preconditions' compile 'org.slf4j:slf4j-api' + testCompile project(':dialogue-client-test-lib') testCompile 'junit:junit' testCompile 'org.assertj:assertj-core' testCompile 'org.immutables:value::annotations' 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 1cdb8d78c..92d51161b 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 @@ -28,6 +28,7 @@ import com.palantir.conjure.java.api.errors.ServiceException; import com.palantir.dialogue.BodySerDe; import com.palantir.dialogue.RequestBody; +import com.palantir.dialogue.TestResponse; import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 30070cd5f..1335aa5a0 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -30,11 +30,13 @@ import com.google.common.util.concurrent.SettableFuture; import com.palantir.dialogue.BodySerDe; import com.palantir.dialogue.Channel; +import com.palantir.dialogue.CloseRecordingInputStream; import com.palantir.dialogue.Deserializer; import com.palantir.dialogue.Endpoint; import com.palantir.dialogue.Request; import com.palantir.dialogue.RequestBody; import com.palantir.dialogue.Response; +import com.palantir.dialogue.TestResponse; import java.io.IOException; import java.io.InputStream; import java.util.Optional; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index 39b303889..4d0b329d5 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -33,6 +33,7 @@ import com.palantir.conjure.java.api.errors.UnknownRemoteException; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.Response; +import com.palantir.dialogue.TestResponse; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import java.io.ByteArrayInputStream; From 55aaab67d48c18145716594752c3ac4ab2448b82 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:01:49 +0000 Subject: [PATCH 19/38] Smaller diff --- .../java/dialogue/serde/ConjureBodySerDe.java | 10 ++-- .../java/dialogue/serde/ErrorDecoder.java | 47 ++++++------------- .../java/dialogue/serde/ErrorDecoderTest.java | 13 ----- 3 files changed, 18 insertions(+), 52 deletions(-) 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 7bfda2aa7..461c0958f 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 @@ -198,7 +198,7 @@ private static final class EncodingDeserializerRegistry implements Deserializ EncodingDeserializerRegistry(List encodings, ErrorDecoder errorDecoder, TypeMarker token) { this.encodings = encodings.stream() - .map(encoding -> EncodingDeserializerContainer.of(encoding, token)) + .map(encoding -> new EncodingDeserializerContainer<>(encoding, token)) .collect(ImmutableList.toImmutableList()); this.errorDecoder = errorDecoder; this.token = token; @@ -279,13 +279,9 @@ private static final class EncodingDeserializerContainer { private final Encoding encoding; private final Encoding.Deserializer deserializer; - EncodingDeserializerContainer(Encoding encoding, Encoding.Deserializer deserializer) { + EncodingDeserializerContainer(Encoding encoding, TypeMarker token) { this.encoding = encoding; - this.deserializer = deserializer; - } - - static EncodingDeserializerContainer of(Encoding encoding, TypeMarker token) { - return new EncodingDeserializerContainer<>(encoding, encoding.deserializer(token)); + this.deserializer = encoding.deserializer(token); } @Override diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java index 22880e4eb..f55fbd1da 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java @@ -30,8 +30,6 @@ import java.nio.charset.StandardCharsets; import java.util.Optional; import javax.ws.rs.core.HttpHeaders; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Extracts and returns a {@link RemoteException} from an {@link Response}. @@ -42,7 +40,6 @@ enum ErrorDecoder { INSTANCE; - private static final Logger log = LoggerFactory.getLogger(ErrorDecoder.class); private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); boolean isError(Response response) { @@ -53,40 +50,26 @@ RemoteException decode(Response response) { // TODO(rfink): What about HTTP/101 switching protocols? // TODO(rfink): What about HEAD requests? - InputStream inputStream = response.body(); + String body; try { - String body; - try { - body = toString(inputStream); - } catch (NullPointerException | IOException e) { - UnknownRemoteException exception = new UnknownRemoteException(response.code(), ""); - exception.initCause(e); - throw exception; - } - - Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { - try { - SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); - return new RemoteException(serializableError, response.code()); - } catch (Exception e) { - throw new UnknownRemoteException(response.code(), body); - } - } + body = toString(response.body()); + } catch (NullPointerException | IOException e) { + UnknownRemoteException exception = new UnknownRemoteException(response.code(), ""); + exception.initCause(e); + throw exception; + } - throw new UnknownRemoteException(response.code(), body); - } finally { - try { - inputStream.close(); - } catch (RuntimeException | IOException e) { - log.warn("Failed to close InputStream", e); - } + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + if (contentType.isPresent() && Encodings.matchesContentType("application/json", contentType.get())) { try { - response.close(); - } catch (RuntimeException e) { - log.warn("Failed to close Response", e); + SerializableError serializableError = MAPPER.readValue(body, SerializableError.class); + return new RemoteException(serializableError, response.code()); + } catch (Exception e) { + throw new UnknownRemoteException(response.code(), body); } } + + throw new UnknownRemoteException(response.code(), body); } private static String toString(InputStream body) throws IOException { diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index 4d0b329d5..dcbdd6bd7 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -33,7 +33,6 @@ import com.palantir.conjure.java.api.errors.UnknownRemoteException; import com.palantir.conjure.java.serialization.ObjectMappers; import com.palantir.dialogue.Response; -import com.palantir.dialogue.TestResponse; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import java.io.ByteArrayInputStream; @@ -147,18 +146,6 @@ public void handlesJsonWithEncoding() { assertThat(exception.getError().errorName()).isEqualTo(ErrorType.FAILED_PRECONDITION.name()); } - @Test - public void closes_response_and_inputstream() { - TestResponse testResponse = new TestResponse().contentType("application/json"); - assertThatThrownBy(() -> decoder.decode(testResponse)).isInstanceOf(UnknownRemoteException.class); - assertThat(testResponse.body().isClosed()) - .describedAs("Expected inputstream to be closed") - .isTrue(); - assertThat(testResponse.isClosed()) - .describedAs("Body should probably be closed too") - .isTrue(); - } - private static RemoteException encodeAndDecode(Exception exception) { Preconditions.checkArgument(!(exception instanceof ServiceException), "Use SerializableError#forException"); Object error = SerializableError.builder() From 2e7f9922b7aa0f9b3b39775f10bd021d8b7fb660 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:09:51 +0000 Subject: [PATCH 20/38] Restore ContentDecodingChannel --- .../palantir/dialogue/hc4/ApacheHttpClientChannels.java | 4 ++-- .../com/palantir/verification/BinaryReturnTypeTest.java | 9 ++++++--- .../java/com/palantir/dialogue/core/DialogueChannel.java | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java b/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java index f08bcd298..f5957b8f0 100644 --- a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java +++ b/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java @@ -161,8 +161,8 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf .disableConnectionState() // Match okhttp behavior disabling cookies .disableCookieManagement() - // // Dialogue handles content-compression with ContentDecodingChannel - // .disableContentCompression() + // Dialogue handles content-compression with ContentDecodingChannel + .disableContentCompression() .setSSLSocketFactory(sslSocketFactory) .setDefaultCredentialsProvider(NullCredentialsProvider.INSTANCE) .setTargetAuthenticationStrategy(NullAuthenticationStrategy.INSTANCE) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index 8ad96ab28..d56919798 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -41,6 +41,7 @@ import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Paths; +import java.time.Duration; import java.util.Optional; import java.util.zip.GZIPOutputStream; import okhttp3.mockwebserver.MockResponse; @@ -92,7 +93,7 @@ public void handleRequest(HttpServerExchange exchange) throws IOException { /** I made two tests for the same thing because I wasn't 100% sure I'd set the server up right. */ @Test - @Ignore // I don't know why this is hanging... + @Ignore // don't know what's going on here public void mockwebserver() throws IOException { try (MockWebServer server = new MockWebServer()) { @@ -103,8 +104,7 @@ public void mockwebserver() throws IOException { .setResponseCode(200) .setHeader("Content-Type", "application/octet-stream") .setHeader("Content-Encoding", "gzip") - .setBody(okioBuffer(gzipCompress("Hello, world"))) - .removeHeader("Content-Length")); + .setBody(okioBuffer(gzipCompress("Hello, world")))); // ApacheHttpClientChannels.CloseableClient apache = // ApacheHttpClientChannels.createCloseableHttpClient(clientConf(uri), "foo"); @@ -132,6 +132,9 @@ private static ClientConfiguration clientConf(String uri) { .from(ClientConfigurations.of(ServiceConfiguration.builder() .addUris(uri) .security(SSL_CONFIG) + .readTimeout(Duration.ofSeconds(1)) + .writeTimeout(Duration.ofSeconds(1)) + .connectTimeout(Duration.ofSeconds(1)) .build())) .userAgent(USER_AGENT) .build(); diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java index c973ce630..64b1a7e67 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java @@ -221,7 +221,7 @@ private static Channel wrap( channel = retryingChannel(channel, channelName, conf, scheduler, random); channel = new UserAgentChannel(channel, conf.userAgent().get()); channel = new DeprecationWarningChannel(channel, clientMetrics); - // channel = new ContentDecodingChannel(channel); + channel = new ContentDecodingChannel(channel); channel = new NeverThrowChannel(channel); channel = new TracedChannel(channel, "Dialogue-request"); channel = new ActiveRequestInstrumentationChannel(channel, channelName, "processing", clientMetrics); From 78a4edfe55c0667546fe901288affbdf2f026686 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:38:30 +0000 Subject: [PATCH 21/38] Closing binary InputStreams triggers Response to close --- .../java/dialogue/serde/BinaryEncoding.java | 36 +++++++-- .../java/dialogue/serde/ConjureBodySerDe.java | 10 ++- .../conjure/java/dialogue/serde/Encoding.java | 3 +- .../java/dialogue/serde/Encodings.java | 8 +- .../dialogue/serde/ForwardingInputStream.java | 73 +++++++++++++++++++ .../dialogue/serde/ConjureBodySerDeTest.java | 3 +- .../dialogue/serde/DefaultClientsTest.java | 26 ++++--- .../java/dialogue/serde/EncodingsTest.java | 4 +- 8 files changed, 139 insertions(+), 24 deletions(-) create mode 100644 dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java index f3bb20936..06a93105d 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java @@ -19,6 +19,8 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeIllegalStateException; +import java.io.Closeable; +import java.io.IOException; import java.io.InputStream; import java.util.Optional; @@ -71,9 +73,9 @@ enum OptionalInputStreamDeserializer implements Deserializer deserialize(InputStream input) { - // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! - return Optional.of(input); + public Optional deserialize(InputStream input, Closeable response) { + // intentionally not closing just yet, otherwise users wouldn't be able to get any data out of it! + return Optional.of(new ResponseClosingInputStream(input, response)); } @Override @@ -86,9 +88,9 @@ enum InputStreamDeserializer implements Deserializer { INSTANCE; @Override - public InputStream deserialize(InputStream input) { - // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! - return input; + public InputStream deserialize(InputStream input, Closeable response) { + // intentionally not closing just yet, otherwise users wouldn't be able to get any data out of it! + return new ResponseClosingInputStream(input, response); } @Override @@ -96,4 +98,26 @@ public String toString() { return "InputStreamDeserializer{}"; } } + + static class ResponseClosingInputStream extends ForwardingInputStream { + private final InputStream delegate; + private final Closeable response; + + ResponseClosingInputStream(InputStream delegate, Closeable response) { + this.delegate = delegate; + this.response = response; + } + + @Override + InputStream delegate() { + return delegate; + } + + @Override + public void close() throws IOException { + // TODO(dfox): try-catch? + super.close(); + response.close(); + } + } } 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 461c0958f..82406c1b2 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 @@ -31,6 +31,7 @@ import com.palantir.logsafe.UnsafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -234,7 +235,7 @@ public T deserialize(Response response) { SafeArg.of("received", response.headers().keySet())); } Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); - return deserializer.deserialize(response.body()); + return deserializer.deserialize(response.body(), response); } @Override @@ -258,12 +259,17 @@ Encoding.Deserializer getResponseDeserializer(String contentType) { private Encoding.Deserializer throwingDeserializer(String contentType) { return new Encoding.Deserializer() { @Override - public T deserialize(InputStream input) { + public T deserialize(InputStream input, Closeable response) { try { input.close(); } catch (RuntimeException | IOException e) { // empty } + try { + response.close(); + } catch (RuntimeException | IOException e) { + // empty + } throw new SafeRuntimeException( "Unsupported Content-Type", SafeArg.of("received", 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 b4e4fc695..edddd6988 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,6 +17,7 @@ package com.palantir.conjure.java.dialogue.serde; import com.palantir.dialogue.TypeMarker; +import java.io.Closeable; import java.io.InputStream; import java.io.OutputStream; @@ -63,7 +64,7 @@ interface Deserializer { *

Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. */ - T deserialize(InputStream input); + T deserialize(InputStream input, Closeable response); } 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 bd0816c42..abfbe3a4f 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 @@ -26,6 +26,7 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeRuntimeException; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import javax.annotation.Nullable; @@ -61,9 +62,10 @@ public final Serializer serializer(TypeMarker type) { @Override public final Deserializer deserializer(TypeMarker type) { ObjectReader reader = mapper.readerFor(mapper.constructType(type.getType())); - return input -> { - try (InputStream inputStream = input) { - T value = reader.readValue(inputStream); + return (InputStream input, Closeable response) -> { + try (InputStream closeMe = input; + Closeable closeMeToo = response) { + T value = reader.readValue(input); // Bad input should result in a 4XX response status, throw IAE rather than NPE. Preconditions.checkArgument(value != null, "cannot deserialize a JSON null value"); return value; diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java new file mode 100644 index 000000000..b7cb864a3 --- /dev/null +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java @@ -0,0 +1,73 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import com.google.common.annotations.VisibleForTesting; +import java.io.IOException; +import java.io.InputStream; + +abstract class ForwardingInputStream extends InputStream { + + /** All methods delegate to this instance. */ + @VisibleForTesting + abstract InputStream delegate(); + + @Override + public int read() throws IOException { + return delegate().read(); + } + + @Override + public int read(byte[] bytes) throws IOException { + return delegate().read(bytes); + } + + @Override + public int read(byte[] bytes, int off, int len) throws IOException { + return delegate().read(bytes, off, len); + } + + @Override + public long skip(long num) throws IOException { + return delegate().skip(num); + } + + @Override + public int available() throws IOException { + return delegate().available(); + } + + @Override + public void close() throws IOException { + delegate().close(); + } + + @Override + public void mark(int readlimit) { + delegate().mark(readlimit); + } + + @Override + public void reset() throws IOException { + delegate().reset(); + } + + @Override + public boolean markSupported() { + return delegate().markSupported(); + } +} 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 92d51161b..ae5544644 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 @@ -33,6 +33,7 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.util.Optional; @@ -216,7 +217,7 @@ public Serializer serializer(TypeMarker _type) { @Override @SuppressWarnings("unchecked") public Deserializer deserializer(TypeMarker type) { - return input -> { + return (InputStream input, Closeable response) -> { Preconditions.checkArgument(TYPE.equals(type), "This stub encoding only supports String"); return (T) getContentType(); }; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 1335aa5a0..ddcc15712 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -124,13 +124,14 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { TestResponse testResponse = new TestResponse().contentType("application/octet-stream"); responseFuture.set(testResponse); - try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) Futures.getUnchecked(future)) { + try (InputStream inputStream = Futures.getUnchecked(future)) { assertThat(inputStream.available()) .describedAs("Content should be empty") .isEqualTo(0); - inputStream.assertNotClosed(); + asCloseRecording(inputStream).assertNotClosed(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): what do we do with the actual response at this point??") + .describedAs("It's ok for the Response to remain open for now, it'll be closed when the user " + + "closes the InputStream") .isFalse(); } @@ -138,8 +139,8 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): I think this should magically be closed by now") - .isFalse(); + .describedAs("Closing the InputStream also closed the Response") + .isTrue(); } @Test @@ -153,13 +154,14 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc responseFuture.set(testResponse); Optional maybeInputStream = Futures.getUnchecked(future); - try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) maybeInputStream.get()) { + try (InputStream inputStream = maybeInputStream.get()) { assertThat(inputStream.available()) .describedAs("Content should be empty") .isEqualTo(0); - inputStream.assertNotClosed(); + asCloseRecording(inputStream).assertNotClosed(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): what do we do with the actual response at this point??") + .describedAs("It's ok for the Response to remain open for now, it'll be closed when the user " + + "closes the InputStream") .isFalse(); } @@ -167,8 +169,8 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): I think this should magically be closed by now") - .isFalse(); + .describedAs("Closing the InputStream also closed the Response") + .isTrue(); } @Test @@ -186,4 +188,8 @@ public void testCallClosesRequestOnCompletion_failure() { assertThat(result).isDone(); verify(body).close(); } + + private static CloseRecordingInputStream asCloseRecording(InputStream inputStream) { + return (CloseRecordingInputStream) ((ForwardingInputStream) inputStream).delegate(); + } } 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 28405c675..cb7758254 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 @@ -79,6 +79,8 @@ private void serialize(Object object, OutputStream stream) throws IOException { } private T deserialize(InputStream stream, TypeMarker token) throws IOException { - return json.deserializer(token).deserialize(stream); + return json.deserializer(token).deserialize(stream, () -> { + // empty + }); } } From bb33dfc6a670a19268e81f96847a79230f857ddb Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:50:01 +0000 Subject: [PATCH 22/38] accept revapi --- .palantir/revapi.yml | 7 +++++++ .../com/palantir/conjure/java/dialogue/serde/Encoding.java | 6 ++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 73ec36989..2906c6830 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -252,3 +252,10 @@ acceptedBreaks: old: null new: "method void com.palantir.dialogue.RequestBody::close()" justification: "internal interface not for extension" + "1.2.0": + com.palantir.dialogue:dialogue-serde: + - code: "java.method.numberOfParametersChanged" + old: "method T com.palantir.conjure.java.dialogue.serde.Encoding.Deserializer::deserialize(java.io.InputStream)" + new: "method T com.palantir.conjure.java.dialogue.serde.Encoding.Deserializer::deserialize(java.io.InputStream,\ + \ java.io.Closeable)" + justification: "There are no known custom Deserializers in the wild yet" 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 edddd6988..b9b1af0eb 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 @@ -58,8 +58,10 @@ interface Deserializer { /** * Reads a serialized type-{@link T} object representation from the given input stream and returns the - * corresponding object. Implementations should read the entire input stream and must close it (unless they - * return the raw InputStream, e.g. for a binary response type). + * corresponding object. + * + *

Implementations must close the {@link InputStream} and the {@code Closeable response}, except in the + * case that an InputStream is returned to the user, e.g. for a binary response type. * *

Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. From cd15fa729fab6048e07dcbff04ea8d4e9a5aa47d Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 03:51:32 +0000 Subject: [PATCH 23/38] rename --- .../conjure/java/dialogue/serde/BinaryEncoding.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java index 06a93105d..61226e043 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java @@ -100,23 +100,23 @@ public String toString() { } static class ResponseClosingInputStream extends ForwardingInputStream { - private final InputStream delegate; + private final InputStream inputStream; private final Closeable response; - ResponseClosingInputStream(InputStream delegate, Closeable response) { - this.delegate = delegate; + ResponseClosingInputStream(InputStream inputStream, Closeable response) { + this.inputStream = inputStream; this.response = response; } @Override InputStream delegate() { - return delegate; + return inputStream; } @Override public void close() throws IOException { // TODO(dfox): try-catch? - super.close(); + inputStream.close(); response.close(); } } From 2c3ff0086478d3900c075e147b8c57e711d29fcd Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 04:02:02 +0000 Subject: [PATCH 24/38] Add generated changelog entries --- changelog/@unreleased/pr-570.v2.yml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/@unreleased/pr-570.v2.yml diff --git a/changelog/@unreleased/pr-570.v2.yml b/changelog/@unreleased/pr-570.v2.yml new file mode 100644 index 000000000..229a767ab --- /dev/null +++ b/changelog/@unreleased/pr-570.v2.yml @@ -0,0 +1,7 @@ +type: fix +fix: + description: Conjure endpoints returning `binary` or `optional` now correctly + return unclosed InputStreams. Users must be careful to close these InputStreams, + otherwise resources will be leaked. + links: + - https://github.com/palantir/dialogue/pull/570 From 4f9431e2cfcd6a3d89298c7937c65943661adeec Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 04:02:02 +0000 Subject: [PATCH 25/38] Move two tests --- .../dialogue/serde/BinaryEncodingTest.java | 72 +++++++++++++++++++ .../dialogue/serde/ConjureBodySerDeTest.java | 24 ------- 2 files changed, 72 insertions(+), 24 deletions(-) create mode 100644 dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java new file mode 100644 index 000000000..b19139bb9 --- /dev/null +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java @@ -0,0 +1,72 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.dialogue.serde; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.collect.ImmutableList; +import com.palantir.dialogue.BodySerDe; +import com.palantir.dialogue.CloseRecordingInputStream; +import com.palantir.dialogue.TestResponse; +import java.io.IOException; +import java.io.InputStream; +import java.util.Optional; +import org.junit.Test; + +public class BinaryEncodingTest { + + @Test + public void testBinary() throws IOException { + TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); + BodySerDe serializers = new ConjureBodySerDe( + ImmutableList.of(WeightedEncoding.of(new ConjureBodySerDeTest.StubEncoding("application/json")))); + InputStream deserialized = serializers.inputStreamDeserializer().deserialize(response); + assertThat(deserialized.available()).isEqualTo(0); + CloseRecordingInputStream rawInputStream = response.body(); + rawInputStream.assertNotClosed(); + assertThat(response.isClosed()) + .describedAs("response is unclosed initially") + .isFalse(); + + deserialized.close(); + assertThat(response.isClosed()) + .describedAs("closing the InputStream magically closes the body too") + .isTrue(); + } + + @Test + public void testBinary_optional_present() throws IOException { + TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); + BodySerDe serializers = new ConjureBodySerDe( + ImmutableList.of(WeightedEncoding.of(new ConjureBodySerDeTest.StubEncoding("application/json")))); + Optional maybe = + serializers.optionalInputStreamDeserializer().deserialize(response); + assertThat(maybe).isPresent(); + InputStream deserialized = maybe.get(); + assertThat(deserialized.available()).isEqualTo(0); + CloseRecordingInputStream rawInputStream = response.body(); + rawInputStream.assertNotClosed(); + assertThat(response.isClosed()) + .describedAs("response is unclosed initially") + .isFalse(); + + deserialized.close(); + assertThat(response.isClosed()) + .describedAs("closing the InputStream magically closes the body too") + .isTrue(); + } +} 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 ae5544644..b6add3783 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 @@ -161,30 +161,6 @@ public void testErrorsDecoded() { .isTrue(); } - @Test - public void testBinary() throws IOException { - TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); - BodySerDe serializers = - new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); - assertThat(serializers.inputStreamDeserializer().deserialize(response).available()) - .isEqualTo(0); - response.body().assertNotClosed(); - assertThat(response.isClosed()).describedAs("TODO ???").isFalse(); - } - - @Test - public void testBinary_optional_present() throws IOException { - TestResponse response = new TestResponse().code(200).contentType("application/octet-stream"); - BodySerDe serializers = - new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(new StubEncoding("application/json")))); - Optional maybe = - serializers.optionalInputStreamDeserializer().deserialize(response); - assertThat(maybe).isPresent(); - assertThat(maybe.get().available()).isEqualTo(0); - response.body().assertNotClosed(); - assertThat(response.isClosed()).describedAs("TODO ???").isFalse(); - } - @Test public void testBinary_optional_empty() { TestResponse response = new TestResponse().code(204); From 1533fa7b81a87b35041379f8cf749afd05812fc8 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 14:37:26 +0000 Subject: [PATCH 26/38] Delete mockwebserver stuff --- dialogue-client-verifier/build.gradle | 1 - .../verification/BinaryReturnTypeTest.java | 120 ++++++------------ 2 files changed, 36 insertions(+), 85 deletions(-) diff --git a/dialogue-client-verifier/build.gradle b/dialogue-client-verifier/build.gradle index 2ec828f40..8866846ee 100644 --- a/dialogue-client-verifier/build.gradle +++ b/dialogue-client-verifier/build.gradle @@ -37,7 +37,6 @@ dependencies { testImplementation 'org.apache.commons:commons-lang3' testImplementation 'com.palantir.conjure.java.runtime:keystores' testImplementation 'io.undertow:undertow-core' - testImplementation 'com.squareup.okhttp3:mockwebserver' testRuntimeOnly 'org.apache.logging.log4j:log4j-slf4j-impl' testRuntimeOnly 'org.apache.logging.log4j:log4j-core' diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index d56919798..277e31b49 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -23,17 +23,15 @@ import com.google.common.util.concurrent.ListenableFuture; import com.palantir.conjure.java.api.config.service.ServiceConfiguration; import com.palantir.conjure.java.api.config.service.UserAgent; -import com.palantir.conjure.java.api.config.service.UserAgents; import com.palantir.conjure.java.api.config.ssl.SslConfiguration; import com.palantir.conjure.java.client.config.ClientConfiguration; import com.palantir.conjure.java.client.config.ClientConfigurations; import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; -import com.palantir.dialogue.Channel; import com.palantir.dialogue.example.SampleServiceAsync; import com.palantir.dialogue.hc4.ApacheHttpClientChannels; import io.undertow.Undertow; import io.undertow.server.HttpHandler; -import io.undertow.server.HttpServerExchange; +import io.undertow.server.handlers.BlockingHandler; import io.undertow.util.Headers; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -44,87 +42,54 @@ import java.time.Duration; import java.util.Optional; import java.util.zip.GZIPOutputStream; -import okhttp3.mockwebserver.MockResponse; -import okhttp3.mockwebserver.MockWebServer; -import okio.Buffer; -import org.junit.Ignore; +import org.junit.After; +import org.junit.Before; import org.junit.Test; public class BinaryReturnTypeTest { - private static final UserAgent USER_AGENT = UserAgent.of(UserAgent.Agent.of("foo", "1.0.0")); + private static final UserAgent USER_AGENT = UserAgent.of(UserAgent.Agent.of("BinaryReturnTypeTest", "0.0.0")); private static final SslConfiguration SSL_CONFIG = SslConfiguration.of( Paths.get("../dialogue-core/src/test/resources/trustStore.jks"), - Paths.get("../dialogue-core/src/test" + "/resources/keyStore.jks"), + Paths.get("../dialogue-core/src/test/resources/keyStore.jks"), "keystore"); - @Test - public void undertow_server() { - Undertow undertow = Undertow.builder() - .addHttpListener(0, "localhost", new HttpHandler() { - @Override - public void handleRequest(HttpServerExchange exchange) throws IOException { - exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); - exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); - exchange.dispatch(ex -> { - ex.startBlocking(); - ex.getOutputStream().write(gzipCompress("Hello, world")); - }); - } - }) - .build(); - try { - undertow.start(); - - String uri = getUri(undertow); - - SampleServiceAsync client = SampleServiceAsync.of( - smartChannel(uri), DefaultConjureRuntime.builder().build()); - ListenableFuture> future = client.getOptionalBinary(); - - Optional maybeBinary = Futures.getUnchecked(future); + private Undertow undertow; + private HttpHandler undertowHandler; - assertThat(maybeBinary).isPresent(); - assertThat(maybeBinary.get()) - .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); - } finally { - undertow.stop(); - } + @Before + public void before() { + undertow = Undertow.builder() + .addHttpListener(0, "localhost", new BlockingHandler(exchange -> undertowHandler.handleRequest(exchange))) + .build(); + undertow.start(); } - /** I made two tests for the same thing because I wasn't 100% sure I'd set the server up right. */ @Test - @Ignore // don't know what's going on here - public void mockwebserver() throws IOException { - try (MockWebServer server = new MockWebServer()) { - - server.start(); - String uri = "http://localhost:" + server.getPort(); - - server.enqueue(new MockResponse() - .setResponseCode(200) - .setHeader("Content-Type", "application/octet-stream") - .setHeader("Content-Encoding", "gzip") - .setBody(okioBuffer(gzipCompress("Hello, world")))); - - // ApacheHttpClientChannels.CloseableClient apache = - // ApacheHttpClientChannels.createCloseableHttpClient(clientConf(uri), "foo"); - // Channel dumbChannel = ApacheHttpClientChannels.createSingleUri(uri, apache); - // - // SampleServiceAsync dumbClient = SampleServiceAsync.of( - // dumbChannel, DefaultConjureRuntime.builder().build()); - SampleServiceAsync client = SampleServiceAsync.of( - smartChannel(uri), DefaultConjureRuntime.builder().build()); - - ListenableFuture> future = client.getOptionalBinary(); + public void conjure_generated_interface_with_optional_binary_return_type_and_gzip() { + undertowHandler = exchange -> { + exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); + exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); + exchange.getOutputStream().write(gzipCompress("Hello, world")); + }; + + SampleServiceAsync client = SampleServiceAsync.of( + ApacheHttpClientChannels.create(clientConf(getUri(undertow))), + DefaultConjureRuntime.builder().build()); + + ListenableFuture> future = client.getOptionalBinary(); + Optional maybeBinary = Futures.getUnchecked(future); + + assertThat(maybeBinary).isPresent(); + assertThat(maybeBinary.get()).hasSameContentAs(asInputStream("Hello, world")); + } - Optional maybeBinary = Futures.getUnchecked(future); + @After + public void after() { + undertow.stop(); + } - assertThat(maybeBinary).isPresent(); - try (InputStream actual = maybeBinary.get()) { - assertThat(actual) - .hasSameContentAs(new ByteArrayInputStream("Hello, world".getBytes(StandardCharsets.UTF_8))); - } - } + private static ByteArrayInputStream asInputStream(String string) { + return new ByteArrayInputStream(string.getBytes(StandardCharsets.UTF_8)); } private static ClientConfiguration clientConf(String uri) { @@ -149,19 +114,6 @@ private static byte[] gzipCompress(String stringToCompress) throws IOException { } } - private static Buffer okioBuffer(byte[] byteArray) { - Buffer buffer = new Buffer(); - buffer.read(byteArray); - return buffer; - } - - private static Channel smartChannel(String address) { - return ApacheHttpClientChannels.create(ClientConfiguration.builder() - .userAgent(UserAgents.parse("FooTest/0.0.0")) - .from(clientConf(address)) - .build()); - } - private static String getUri(Undertow undertow) { Undertow.ListenerInfo listenerInfo = Iterables.getOnlyElement(undertow.getListenerInfo()); return String.format("%s:/%s", listenerInfo.getProtcol(), listenerInfo.getAddress()); From cc6dd8905482b9c7f0a6c635af5d3e2a1c97a531 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 14:38:11 +0000 Subject: [PATCH 27/38] Move defensiveness to ConjureBodySerde --- .../java/dialogue/serde/ConjureBodySerDe.java | 45 ++++++++++--------- .../java/dialogue/serde/DefaultClients.java | 14 +----- 2 files changed, 25 insertions(+), 34 deletions(-) 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 82406c1b2..9f0916f62 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 @@ -211,31 +211,34 @@ private static final class EncodingDeserializerRegistry implements Deserializ @Override public T deserialize(Response response) { - if (errorDecoder.isError(response)) { - try { + boolean closeResponse = true; + try { + if (errorDecoder.isError(response)) { throw errorDecoder.decode(response); - } finally { - response.close(); - } - } else if (response.code() == 204) { - response.close(); - if (!isOptionalType) { - throw new SafeRuntimeException( - "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); - } else { - return TypeMarkers.getEmptyOptional(token); + } else if (response.code() == 204) { + if (!isOptionalType) { + throw new SafeRuntimeException( + "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); + } else { + return TypeMarkers.getEmptyOptional(token); + } } - } - Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); - if (!contentType.isPresent()) { - response.close(); - throw new SafeIllegalArgumentException( - "Response is missing Content-Type header", - SafeArg.of("received", response.headers().keySet())); + Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); + if (!contentType.isPresent()) { + throw new SafeIllegalArgumentException( + "Response is missing Content-Type header", + SafeArg.of("received", response.headers().keySet())); + } + Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); + // deserializer is responsible for closing the response + closeResponse = false; + return deserializer.deserialize(response.body(), response); + } finally { + if (closeResponse) { + response.close(); + } } - Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); - return deserializer.deserialize(response.body(), response); } @Override diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java index 83bb8e9bc..c60b85cf8 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java @@ -57,19 +57,7 @@ public ListenableFuture call( Request outgoingRequest = accepts.isPresent() ? accepting(request, accepts.get()) : request; ListenableFuture response = closeRequestBodyOnCompletion(channel.execute(endpoint, outgoingRequest), outgoingRequest); - return Futures.transform( - response, - resp -> { - try { - return deserializer.deserialize(resp); - } catch (RuntimeException e) { - if (resp != null) { - resp.close(); // extra safety net, just in case something unexpected throws - } - throw e; - } - }, - MoreExecutors.directExecutor()); + return Futures.transform(response, deserializer::deserialize, MoreExecutors.directExecutor()); } private static ListenableFuture closeRequestBodyOnCompletion( From 6711e940343cbb3b4703a55052f920655fbf5b30 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 14:41:37 +0000 Subject: [PATCH 28/38] Also sanity check the blocking interface --- .../verification/BinaryReturnTypeTest.java | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index 277e31b49..670f8dfc8 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -28,6 +28,7 @@ import com.palantir.conjure.java.client.config.ClientConfigurations; import com.palantir.conjure.java.dialogue.serde.DefaultConjureRuntime; import com.palantir.dialogue.example.SampleServiceAsync; +import com.palantir.dialogue.example.SampleServiceBlocking; import com.palantir.dialogue.hc4.ApacheHttpClientChannels; import io.undertow.Undertow; import io.undertow.server.HttpHandler; @@ -59,30 +60,51 @@ public class BinaryReturnTypeTest { @Before public void before() { undertow = Undertow.builder() - .addHttpListener(0, "localhost", new BlockingHandler(exchange -> undertowHandler.handleRequest(exchange))) + .addHttpListener( + 0, "localhost", new BlockingHandler(exchange -> undertowHandler.handleRequest(exchange))) .build(); undertow.start(); } @Test - public void conjure_generated_interface_with_optional_binary_return_type_and_gzip() { - undertowHandler = exchange -> { - exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); - exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); - exchange.getOutputStream().write(gzipCompress("Hello, world")); - }; + public void conjure_generated_async_interface_with_optional_binary_return_type_and_gzip() { + setBinaryGzipResponse("Hello, world"); + SampleServiceAsync client = sampleServiceAsync(); + + ListenableFuture> future = client.getOptionalBinary(); + Optional maybeBinary = Futures.getUnchecked(future); + + assertThat(maybeBinary).isPresent(); + assertThat(maybeBinary.get()).hasSameContentAs(asInputStream("Hello, world")); + } - SampleServiceAsync client = SampleServiceAsync.of( + @Test + public void conjure_generated_blocking_interface_with_optional_binary_return_type_and_gzip() { + setBinaryGzipResponse("Hello, world"); + SampleServiceBlocking client = SampleServiceBlocking.of( ApacheHttpClientChannels.create(clientConf(getUri(undertow))), DefaultConjureRuntime.builder().build()); - ListenableFuture> future = client.getOptionalBinary(); - Optional maybeBinary = Futures.getUnchecked(future); + Optional maybeBinary = client.getOptionalBinary(); assertThat(maybeBinary).isPresent(); assertThat(maybeBinary.get()).hasSameContentAs(asInputStream("Hello, world")); } + private void setBinaryGzipResponse(String stringToCompress) { + undertowHandler = exchange -> { + exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); + exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); + exchange.getOutputStream().write(gzipCompress(stringToCompress)); + }; + } + + private SampleServiceAsync sampleServiceAsync() { + return SampleServiceAsync.of( + ApacheHttpClientChannels.create(clientConf(getUri(undertow))), + DefaultConjureRuntime.builder().build()); + } + @After public void after() { undertow.stop(); From 39295caf8190cf143c8f96b051c5f8a4230c7621 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 14:42:50 +0000 Subject: [PATCH 29/38] Minimize diff --- .../palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 9f0916f62..ecf3fbc2a 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 @@ -219,9 +219,8 @@ public T deserialize(Response response) { if (!isOptionalType) { throw new SafeRuntimeException( "Unable to deserialize non-optional response type from 204", SafeArg.of("type", token)); - } else { - return TypeMarkers.getEmptyOptional(token); } + return TypeMarkers.getEmptyOptional(token); } Optional contentType = response.getFirstHeader(HttpHeaders.CONTENT_TYPE); From 3391f57a0dbcf3653bef69f5d9a07731f5863810 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:44:20 +0000 Subject: [PATCH 30/38] stream 3GB --- .../verification/BinaryReturnTypeTest.java | 84 ++++++++++++++++++- 1 file changed, 80 insertions(+), 4 deletions(-) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index 670f8dfc8..f30257a0f 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -18,7 +18,10 @@ import static org.assertj.core.api.Assertions.assertThat; +import com.google.common.base.Stopwatch; import com.google.common.collect.Iterables; +import com.google.common.io.ByteStreams; +import com.google.common.primitives.Ints; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.palantir.conjure.java.api.config.service.ServiceConfiguration; @@ -41,7 +44,9 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Paths; import java.time.Duration; +import java.util.Arrays; import java.util.Optional; +import java.util.concurrent.TimeUnit; import java.util.zip.GZIPOutputStream; import org.junit.After; import org.junit.Before; @@ -81,16 +86,39 @@ public void conjure_generated_async_interface_with_optional_binary_return_type_a @Test public void conjure_generated_blocking_interface_with_optional_binary_return_type_and_gzip() { setBinaryGzipResponse("Hello, world"); - SampleServiceBlocking client = SampleServiceBlocking.of( - ApacheHttpClientChannels.create(clientConf(getUri(undertow))), - DefaultConjureRuntime.builder().build()); - Optional maybeBinary = client.getOptionalBinary(); + Optional maybeBinary = sampleServiceBlocking().getOptionalBinary(); assertThat(maybeBinary).isPresent(); assertThat(maybeBinary.get()).hasSameContentAs(asInputStream("Hello, world")); } + @Test + public void stream_3_gigabytes() throws IOException { + long oneMegabyte = 1000_000; + int megabytes = 3000; + long limit = megabytes * oneMegabyte; + assertThat(limit).isGreaterThan(Integer.MAX_VALUE); + + byte[] sample = new byte[8192]; + Arrays.fill(sample, (byte) 'A'); + + undertowHandler = exchange -> { + exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); + InputStream bigInputStream = repeat(sample, limit); + ByteStreams.copy(bigInputStream, exchange.getOutputStream()); + }; + + Stopwatch sw = Stopwatch.createStarted(); + + InputStream maybeBinary = sampleServiceBlocking().getOptionalBinary().get(); + assertThat(ByteStreams.exhaust(maybeBinary)) + .describedAs("Should receive exactly the number of bytes we sent!") + .isEqualTo(limit); + + System.out.printf("%d MB took %d millis%n", megabytes, sw.elapsed(TimeUnit.MILLISECONDS)); + } + private void setBinaryGzipResponse(String stringToCompress) { undertowHandler = exchange -> { exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); @@ -99,6 +127,12 @@ private void setBinaryGzipResponse(String stringToCompress) { }; } + private SampleServiceBlocking sampleServiceBlocking() { + return SampleServiceBlocking.of( + ApacheHttpClientChannels.create(clientConf(getUri(undertow))), + DefaultConjureRuntime.builder().build()); + } + private SampleServiceAsync sampleServiceAsync() { return SampleServiceAsync.of( ApacheHttpClientChannels.create(clientConf(getUri(undertow))), @@ -140,4 +174,46 @@ private static String getUri(Undertow undertow) { Undertow.ListenerInfo listenerInfo = Iterables.getOnlyElement(undertow.getListenerInfo()); return String.format("%s:/%s", listenerInfo.getProtcol(), listenerInfo.getAddress()); } + + /** Produces a big inputStream by repeating a smaller bytearray sample until limit is reached. */ + private static InputStream repeat(byte[] sample, long limit) { + return new InputStream() { + private long position = 0; + + public int read() { + if (position < limit) { + return sample[(int) (position++ % sample.length)]; + } else { + return -1; + } + } + + @Override + public int read(byte[] outputArray, int off, int len) { + long remainingInStream = limit - position; + if (remainingInStream <= 0) { + return -1; + } + + int numBytesToWrite = (int) Math.min(len, remainingInStream); + int bytesWritten = 0; + while (bytesWritten < numBytesToWrite) { + int sampleIndex = (int) position % sample.length; + int outputIndex = off + bytesWritten; + int chunkSize = Math.min(sample.length - sampleIndex, numBytesToWrite - bytesWritten); + + System.arraycopy(sample, sampleIndex, outputArray, outputIndex, chunkSize); + position += chunkSize; + bytesWritten += chunkSize; + } + + return bytesWritten; + } + + @Override + public int available() { + return Ints.saturatedCast(limit - position); + } + }; + } } From 138f0e14c072e0f0131d80ef0080f8b96b2feac3 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:47:25 +0000 Subject: [PATCH 31/38] Revert "accept revapi" This reverts commit bb33dfc6a670a19268e81f96847a79230f857ddb. --- .palantir/revapi.yml | 7 ------- .../com/palantir/conjure/java/dialogue/serde/Encoding.java | 6 ++---- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 2906c6830..73ec36989 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -252,10 +252,3 @@ acceptedBreaks: old: null new: "method void com.palantir.dialogue.RequestBody::close()" justification: "internal interface not for extension" - "1.2.0": - com.palantir.dialogue:dialogue-serde: - - code: "java.method.numberOfParametersChanged" - old: "method T com.palantir.conjure.java.dialogue.serde.Encoding.Deserializer::deserialize(java.io.InputStream)" - new: "method T com.palantir.conjure.java.dialogue.serde.Encoding.Deserializer::deserialize(java.io.InputStream,\ - \ java.io.Closeable)" - justification: "There are no known custom Deserializers in the wild yet" 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 b9b1af0eb..edddd6988 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 @@ -58,10 +58,8 @@ interface Deserializer { /** * Reads a serialized type-{@link T} object representation from the given input stream and returns the - * corresponding object. - * - *

Implementations must close the {@link InputStream} and the {@code Closeable response}, except in the - * case that an InputStream is returned to the user, e.g. for a binary response type. + * corresponding object. Implementations should read the entire input stream and must close it (unless they + * return the raw InputStream, e.g. for a binary response type). * *

Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. From 68dc8ed3187c6e6659d43d9e03c540fb954d7177 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:48:38 +0000 Subject: [PATCH 32/38] Revert "Closing binary InputStreams triggers Response to close" This reverts commit 78a4edfe55c0667546fe901288affbdf2f026686. --- .../java/dialogue/serde/BinaryEncoding.java | 36 ++------- .../java/dialogue/serde/ConjureBodySerDe.java | 10 +-- .../conjure/java/dialogue/serde/Encoding.java | 3 +- .../java/dialogue/serde/Encodings.java | 8 +- .../dialogue/serde/ForwardingInputStream.java | 73 ------------------- .../dialogue/serde/ConjureBodySerDeTest.java | 3 +- .../dialogue/serde/DefaultClientsTest.java | 26 +++---- .../java/dialogue/serde/EncodingsTest.java | 4 +- 8 files changed, 24 insertions(+), 139 deletions(-) delete mode 100644 dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java index 61226e043..f3bb20936 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java @@ -19,8 +19,6 @@ import com.palantir.dialogue.TypeMarker; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeIllegalStateException; -import java.io.Closeable; -import java.io.IOException; import java.io.InputStream; import java.util.Optional; @@ -73,9 +71,9 @@ enum OptionalInputStreamDeserializer implements Deserializer deserialize(InputStream input, Closeable response) { - // intentionally not closing just yet, otherwise users wouldn't be able to get any data out of it! - return Optional.of(new ResponseClosingInputStream(input, response)); + public Optional deserialize(InputStream input) { + // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! + return Optional.of(input); } @Override @@ -88,9 +86,9 @@ enum InputStreamDeserializer implements Deserializer { INSTANCE; @Override - public InputStream deserialize(InputStream input, Closeable response) { - // intentionally not closing just yet, otherwise users wouldn't be able to get any data out of it! - return new ResponseClosingInputStream(input, response); + public InputStream deserialize(InputStream input) { + // intentionally not closing this, otherwise users wouldn't be able to get any data out of it! + return input; } @Override @@ -98,26 +96,4 @@ public String toString() { return "InputStreamDeserializer{}"; } } - - static class ResponseClosingInputStream extends ForwardingInputStream { - private final InputStream inputStream; - private final Closeable response; - - ResponseClosingInputStream(InputStream inputStream, Closeable response) { - this.inputStream = inputStream; - this.response = response; - } - - @Override - InputStream delegate() { - return inputStream; - } - - @Override - public void close() throws IOException { - // TODO(dfox): try-catch? - inputStream.close(); - response.close(); - } - } } 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 ecf3fbc2a..115be3877 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 @@ -31,7 +31,6 @@ import com.palantir.logsafe.UnsafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; -import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -232,7 +231,7 @@ public T deserialize(Response response) { Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); // deserializer is responsible for closing the response closeResponse = false; - return deserializer.deserialize(response.body(), response); + return deserializer.deserialize(response.body()); } finally { if (closeResponse) { response.close(); @@ -261,17 +260,12 @@ Encoding.Deserializer getResponseDeserializer(String contentType) { private Encoding.Deserializer throwingDeserializer(String contentType) { return new Encoding.Deserializer() { @Override - public T deserialize(InputStream input, Closeable response) { + public T deserialize(InputStream input) { try { input.close(); } catch (RuntimeException | IOException e) { // empty } - try { - response.close(); - } catch (RuntimeException | IOException e) { - // empty - } throw new SafeRuntimeException( "Unsupported Content-Type", SafeArg.of("received", 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 edddd6988..b4e4fc695 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.Closeable; import java.io.InputStream; import java.io.OutputStream; @@ -64,7 +63,7 @@ interface Deserializer { *

Format-related deserialization errors surface as {@link IllegalArgumentException}. Inputs and outputs * must never be null. */ - T deserialize(InputStream input, Closeable response); + 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 abfbe3a4f..bd0816c42 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 @@ -26,7 +26,6 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.exceptions.SafeRuntimeException; -import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import javax.annotation.Nullable; @@ -62,10 +61,9 @@ public final Serializer serializer(TypeMarker type) { @Override public final Deserializer deserializer(TypeMarker type) { ObjectReader reader = mapper.readerFor(mapper.constructType(type.getType())); - return (InputStream input, Closeable response) -> { - try (InputStream closeMe = input; - Closeable closeMeToo = response) { - T value = reader.readValue(input); + return input -> { + try (InputStream inputStream = input) { + T value = reader.readValue(inputStream); // Bad input should result in a 4XX response status, throw IAE rather than NPE. Preconditions.checkArgument(value != null, "cannot deserialize a JSON null value"); return value; diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java deleted file mode 100644 index b7cb864a3..000000000 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ForwardingInputStream.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.conjure.java.dialogue.serde; - -import com.google.common.annotations.VisibleForTesting; -import java.io.IOException; -import java.io.InputStream; - -abstract class ForwardingInputStream extends InputStream { - - /** All methods delegate to this instance. */ - @VisibleForTesting - abstract InputStream delegate(); - - @Override - public int read() throws IOException { - return delegate().read(); - } - - @Override - public int read(byte[] bytes) throws IOException { - return delegate().read(bytes); - } - - @Override - public int read(byte[] bytes, int off, int len) throws IOException { - return delegate().read(bytes, off, len); - } - - @Override - public long skip(long num) throws IOException { - return delegate().skip(num); - } - - @Override - public int available() throws IOException { - return delegate().available(); - } - - @Override - public void close() throws IOException { - delegate().close(); - } - - @Override - public void mark(int readlimit) { - delegate().mark(readlimit); - } - - @Override - public void reset() throws IOException { - delegate().reset(); - } - - @Override - public boolean markSupported() { - return delegate().markSupported(); - } -} 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 b6add3783..9fddf5c61 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 @@ -33,7 +33,6 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; -import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.util.Optional; @@ -193,7 +192,7 @@ public Serializer serializer(TypeMarker _type) { @Override @SuppressWarnings("unchecked") public Deserializer deserializer(TypeMarker type) { - return (InputStream input, Closeable response) -> { + return input -> { Preconditions.checkArgument(TYPE.equals(type), "This stub encoding only supports String"); return (T) getContentType(); }; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index ddcc15712..1335aa5a0 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -124,14 +124,13 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { TestResponse testResponse = new TestResponse().contentType("application/octet-stream"); responseFuture.set(testResponse); - try (InputStream inputStream = Futures.getUnchecked(future)) { + try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) Futures.getUnchecked(future)) { assertThat(inputStream.available()) .describedAs("Content should be empty") .isEqualTo(0); - asCloseRecording(inputStream).assertNotClosed(); + inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) - .describedAs("It's ok for the Response to remain open for now, it'll be closed when the user " - + "closes the InputStream") + .describedAs("TODO(dfox): what do we do with the actual response at this point??") .isFalse(); } @@ -139,8 +138,8 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("Closing the InputStream also closed the Response") - .isTrue(); + .describedAs("TODO(dfox): I think this should magically be closed by now") + .isFalse(); } @Test @@ -154,14 +153,13 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc responseFuture.set(testResponse); Optional maybeInputStream = Futures.getUnchecked(future); - try (InputStream inputStream = maybeInputStream.get()) { + try (CloseRecordingInputStream inputStream = (CloseRecordingInputStream) maybeInputStream.get()) { assertThat(inputStream.available()) .describedAs("Content should be empty") .isEqualTo(0); - asCloseRecording(inputStream).assertNotClosed(); + inputStream.assertNotClosed(); assertThat(testResponse.isClosed()) - .describedAs("It's ok for the Response to remain open for now, it'll be closed when the user " - + "closes the InputStream") + .describedAs("TODO(dfox): what do we do with the actual response at this point??") .isFalse(); } @@ -169,8 +167,8 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("Closing the InputStream also closed the Response") - .isTrue(); + .describedAs("TODO(dfox): I think this should magically be closed by now") + .isFalse(); } @Test @@ -188,8 +186,4 @@ public void testCallClosesRequestOnCompletion_failure() { assertThat(result).isDone(); verify(body).close(); } - - private static CloseRecordingInputStream asCloseRecording(InputStream inputStream) { - return (CloseRecordingInputStream) ((ForwardingInputStream) inputStream).delegate(); - } } 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 cb7758254..28405c675 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 @@ -79,8 +79,6 @@ private void serialize(Object object, OutputStream stream) throws IOException { } private T deserialize(InputStream stream, TypeMarker token) throws IOException { - return json.deserializer(token).deserialize(stream, () -> { - // empty - }); + return json.deserializer(token).deserialize(stream); } } From 0048ab31a81572f7216e05b5db726224ee2deb13 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:52:52 +0000 Subject: [PATCH 33/38] Update test messages --- .../java/dialogue/serde/BinaryEncodingTest.java | 10 ++++++---- .../java/dialogue/serde/DefaultClientsTest.java | 14 ++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java index b19139bb9..1f1c13a34 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java @@ -44,8 +44,9 @@ public void testBinary() throws IOException { deserialized.close(); assertThat(response.isClosed()) - .describedAs("closing the InputStream magically closes the body too") - .isTrue(); + .describedAs( + "Response#close was never called, but no big deal because the body is the only resource worth closing") + .isFalse(); } @Test @@ -66,7 +67,8 @@ public void testBinary_optional_present() throws IOException { deserialized.close(); assertThat(response.isClosed()) - .describedAs("closing the InputStream magically closes the body too") - .isTrue(); + .describedAs( + "Response#close was never called, but no big deal because the body is the only resource worth closing") + .isFalse(); } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 1335aa5a0..9ea6c912b 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -129,16 +129,15 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { .describedAs("Content should be empty") .isEqualTo(0); inputStream.assertNotClosed(); - assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): what do we do with the actual response at this point??") - .isFalse(); + assertThat(testResponse.isClosed()).describedAs("Response").isFalse(); } assertThat(testResponse.body().isClosed()) .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): I think this should magically be closed by now") + .describedAs( + "Response#close was never called, but no big deal because the body is the only resource worth closing") .isFalse(); } @@ -158,16 +157,15 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc .describedAs("Content should be empty") .isEqualTo(0); inputStream.assertNotClosed(); - assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): what do we do with the actual response at this point??") - .isFalse(); + assertThat(testResponse.isClosed()).describedAs("Response").isFalse(); } assertThat(testResponse.body().isClosed()) .describedAs("User has closed it now") .isTrue(); assertThat(testResponse.isClosed()) - .describedAs("TODO(dfox): I think this should magically be closed by now") + .describedAs( + "Response#close was never called, but no big deal because the body is the only resource worth closing") .isFalse(); } From 88a4cbf54b68dd12c8d0ca022da7f610b3608cb7 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:54:06 +0000 Subject: [PATCH 34/38] format --- .../com/palantir/verification/BinaryReturnTypeTest.java | 3 +++ .../conjure/java/dialogue/serde/BinaryEncodingTest.java | 6 ++++-- .../conjure/java/dialogue/serde/ConjureBodySerDeTest.java | 1 - .../conjure/java/dialogue/serde/DefaultClientsTest.java | 6 ++++-- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index f30257a0f..843a6b72f 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -180,6 +180,7 @@ private static InputStream repeat(byte[] sample, long limit) { return new InputStream() { private long position = 0; + @Override public int read() { if (position < limit) { return sample[(int) (position++ % sample.length)]; @@ -188,6 +189,8 @@ public int read() { } } + // this optimized version isn't really necessary, I just wanted to see how fast we could make + // the test go @Override public int read(byte[] outputArray, int off, int len) { long remainingInStream = limit - position; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java index 1f1c13a34..4bd82c2c6 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/BinaryEncodingTest.java @@ -45,7 +45,8 @@ public void testBinary() throws IOException { deserialized.close(); assertThat(response.isClosed()) .describedAs( - "Response#close was never called, but no big deal because the body is the only resource worth closing") + "Response#close was never called, but no big deal because the body is the only resource worth" + + " closing") .isFalse(); } @@ -68,7 +69,8 @@ public void testBinary_optional_present() throws IOException { deserialized.close(); assertThat(response.isClosed()) .describedAs( - "Response#close was never called, but no big deal because the body is the only resource worth closing") + "Response#close was never called, but no big deal because the body is the only resource worth" + + " closing") .isFalse(); } } 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 9fddf5c61..181a476a2 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 @@ -34,7 +34,6 @@ import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; -import java.io.InputStream; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java index 9ea6c912b..1715222bf 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java @@ -137,7 +137,8 @@ public void testBinaryResponse_inputStreamRemainsUnclosed() throws IOException { .isTrue(); assertThat(testResponse.isClosed()) .describedAs( - "Response#close was never called, but no big deal because the body is the only resource worth closing") + "Response#close was never called, but no big deal because the body is the only resource worth" + + " closing") .isFalse(); } @@ -165,7 +166,8 @@ public void testOptionalBinaryResponse_inputStreamRemainsUnclosed() throws IOExc .isTrue(); assertThat(testResponse.isClosed()) .describedAs( - "Response#close was never called, but no big deal because the body is the only resource worth closing") + "Response#close was never called, but no big deal because the body is the only resource worth" + + " closing") .isFalse(); } From 3bcdfc69db1088361db9d29e6981ec6a7257c022 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 15:55:34 +0000 Subject: [PATCH 35/38] log line --- .../palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 115be3877..79994c00e 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 @@ -190,6 +190,7 @@ private static final class EncodingSerializerContainer { private static final class EncodingDeserializerRegistry implements Deserializer { + private static final Logger log = LoggerFactory.getLogger(EncodingDeserializerRegistry.class); private final ImmutableList> encodings; private final ErrorDecoder errorDecoder; private final TypeMarker token; @@ -264,7 +265,7 @@ public T deserialize(InputStream input) { try { input.close(); } catch (RuntimeException | IOException e) { - // empty + log.warn("Failed to close InputStream", e); } throw new SafeRuntimeException( "Unsupported Content-Type", From 98d64db9bda709849f43a516d43c33c304e89dae Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 16:02:25 +0000 Subject: [PATCH 36/38] Extra javadoc --- .../src/main/java/com/palantir/dialogue/Response.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dialogue-target/src/main/java/com/palantir/dialogue/Response.java b/dialogue-target/src/main/java/com/palantir/dialogue/Response.java index 02e77bbb2..f767e1e7d 100644 --- a/dialogue-target/src/main/java/com/palantir/dialogue/Response.java +++ b/dialogue-target/src/main/java/com/palantir/dialogue/Response.java @@ -43,6 +43,9 @@ default Optional getFirstHeader(String header) { * Releases all resources associated with this response. If the {@link #body()} is still open, {@link #close()} * should {@link InputStream#close() close the stream}. * Implementations must not throw, preferring to catch and log internally. + * + * Note that in the case of binary and optional binary endpoints, this method may not be called as the user is + * expected to close the {@link #body()} themselves. */ @Override void close(); From 8a24c6ffd6c0c5254165186cd6e26569a3751d95 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 16:12:27 +0000 Subject: [PATCH 37/38] More defensive about broken encodings --- .../java/dialogue/serde/ConjureBodySerDe.java | 5 ++- .../dialogue/serde/ConjureBodySerDeTest.java | 43 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) 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 79994c00e..64c4f4e16 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 @@ -230,9 +230,10 @@ public T deserialize(Response response) { SafeArg.of("received", response.headers().keySet())); } Encoding.Deserializer deserializer = getResponseDeserializer(contentType.get()); - // deserializer is responsible for closing the response + T deserialized = deserializer.deserialize(response.body()); + // deserializer has taken on responsibility for closing the response body closeResponse = false; - return deserializer.deserialize(response.body()); + return deserialized; } finally { if (closeResponse) { response.close(); 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 181a476a2..554804280 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 @@ -34,6 +34,7 @@ import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.IOException; +import java.io.InputStream; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -172,6 +173,48 @@ public void testBinary_optional_empty() { assertThat(response.isClosed()).describedAs("response should be closed").isTrue(); } + @Test + public void if_deserialize_throws_response_is_still_closed() { + TestResponse response = new TestResponse().code(200).contentType("application/json"); + BodySerDe serializers = new ConjureBodySerDe(ImmutableList.of(WeightedEncoding.of(BrokenEncoding.INSTANCE))); + assertThatThrownBy(() -> serializers.deserializer(TYPE).deserialize(response)) + .isInstanceOf(SafeRuntimeException.class) + .hasMessage("brokenEncoding is broken"); + assertThat(response.body().isClosed()) + .describedAs("inputstream should be closed") + .isTrue(); + assertThat(response.isClosed()).describedAs("response should be closed").isTrue(); + } + + enum BrokenEncoding implements Encoding { + INSTANCE; + + @Override + public Serializer serializer(TypeMarker type) { + throw new UnsupportedOperationException("unimplemented"); + } + + @Override + public Deserializer deserializer(TypeMarker type) { + return new Deserializer() { + @Override + public T deserialize(InputStream _input) { + throw new SafeRuntimeException("brokenEncoding is broken"); + } + }; + } + + @Override + public String getContentType() { + return "application/json"; + } + + @Override + public boolean supportsContentType(String _contentType) { + return true; + } + } + /** Deserializes requests as the configured content type. */ public static final class StubEncoding implements Encoding { From 399eef3d545afe6be3edb46df5e9a93501d7b1d4 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Fri, 27 Mar 2020 16:16:51 +0000 Subject: [PATCH 38/38] sanity check accept-encoding header in tests --- .../java/com/palantir/verification/BinaryReturnTypeTest.java | 5 +++++ .../conjure/java/dialogue/serde/ConjureBodySerDeTest.java | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java index 843a6b72f..33dd4af31 100644 --- a/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java +++ b/dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java @@ -33,6 +33,7 @@ import com.palantir.dialogue.example.SampleServiceAsync; import com.palantir.dialogue.example.SampleServiceBlocking; import com.palantir.dialogue.hc4.ApacheHttpClientChannels; +import com.palantir.logsafe.Preconditions; import io.undertow.Undertow; import io.undertow.server.HttpHandler; import io.undertow.server.handlers.BlockingHandler; @@ -122,6 +123,10 @@ public void stream_3_gigabytes() throws IOException { private void setBinaryGzipResponse(String stringToCompress) { undertowHandler = exchange -> { exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream"); + Preconditions.checkArgument(exchange.getRequestHeaders().contains(Headers.ACCEPT_ENCODING)); + Preconditions.checkArgument(exchange.getRequestHeaders() + .getFirst(Headers.ACCEPT_ENCODING) + .contains("gzip")); exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip"); exchange.getOutputStream().write(gzipCompress(stringToCompress)); }; 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 554804280..19da472a0 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 @@ -190,12 +190,12 @@ enum BrokenEncoding implements Encoding { INSTANCE; @Override - public Serializer serializer(TypeMarker type) { + public Serializer serializer(TypeMarker _type) { throw new UnsupportedOperationException("unimplemented"); } @Override - public Deserializer deserializer(TypeMarker type) { + public Deserializer deserializer(TypeMarker _type) { return new Deserializer() { @Override public T deserialize(InputStream _input) {