Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return unclosed InputStreams for binary and optional<binary> endpoints #570

Merged
merged 40 commits into from
Mar 27, 2020
Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
37d1945
BinaryReturnTypeTest
iamdanfox Mar 26, 2020
8d5565d
Try turning off ContentDecodingChannel entirely
iamdanfox Mar 27, 2020
d24f8ce
make the test green
iamdanfox Mar 27, 2020
1788dee
Merge remote-tracking branch 'origin/develop' into dfox/repro-gzip-bi…
iamdanfox Mar 27, 2020
b758135
CloseableInputStream proves ConjureBodySerde is broken
iamdanfox Mar 27, 2020
4ba8567
Show exactly where the close was called
iamdanfox Mar 27, 2020
a10d2f0
ConjureBodySerde should not be responsible for closing
iamdanfox Mar 27, 2020
37bffd1
Factor out TestResponse
iamdanfox Mar 27, 2020
d249e5c
DEFAULT_ENCODINGS
iamdanfox Mar 27, 2020
e772029
better TestResponse and CloseRecordingInputStream
iamdanfox Mar 27, 2020
443aa53
test binary & optional<binary> responses
iamdanfox Mar 27, 2020
a11d0f9
Renames
iamdanfox Mar 27, 2020
f24f5d3
Minor fix
iamdanfox Mar 27, 2020
9ee901f
ErrorDecoder closes response & inputstream
iamdanfox Mar 27, 2020
8ac1c0b
Update javadoc
iamdanfox Mar 27, 2020
79ac9d1
format & checkstyle
iamdanfox Mar 27, 2020
4c98652
Close out unknowns too
iamdanfox Mar 27, 2020
1f43c4d
Extra safety net, just in case
iamdanfox Mar 27, 2020
4a5c246
Move useful types to :test-lib
iamdanfox Mar 27, 2020
55aaab6
Smaller diff
iamdanfox Mar 27, 2020
2e7f992
Restore ContentDecodingChannel
iamdanfox Mar 27, 2020
78a4edf
Closing binary InputStreams triggers Response to close
iamdanfox Mar 27, 2020
bb33dfc
accept revapi
iamdanfox Mar 27, 2020
cd15fa7
rename
iamdanfox Mar 27, 2020
2c3ff00
Add generated changelog entries
iamdanfox Mar 27, 2020
4f9431e
Move two tests
iamdanfox Mar 27, 2020
1533fa7
Delete mockwebserver stuff
iamdanfox Mar 27, 2020
cc6dd89
Move defensiveness to ConjureBodySerde
iamdanfox Mar 27, 2020
ba557cd
Merge branch 'dfox/repro-gzip-binary-response' of ssh://github.com/pa…
iamdanfox Mar 27, 2020
6711e94
Also sanity check the blocking interface
iamdanfox Mar 27, 2020
39295ca
Minimize diff
iamdanfox Mar 27, 2020
3391f57
stream 3GB
iamdanfox Mar 27, 2020
138f0e1
Revert "accept revapi"
iamdanfox Mar 27, 2020
68dc8ed
Revert "Closing binary InputStreams triggers Response to close"
iamdanfox Mar 27, 2020
0048ab3
Update test messages
iamdanfox Mar 27, 2020
88a4cbf
format
iamdanfox Mar 27, 2020
3bcdfc6
log line
iamdanfox Mar 27, 2020
98d64db
Extra javadoc
iamdanfox Mar 27, 2020
8a24c6f
More defensive about broken encodings
iamdanfox Mar 27, 2020
399eef3
sanity check accept-encoding header in tests
iamdanfox Mar 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-570.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: fix
fix:
description: Conjure endpoints returning `binary` or `optional<binary>` 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* (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.dialogue;

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. */
Copy link
Contributor

Choose a reason for hiding this comment

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

is this true? It looks like close can be called several times, which should work correctly based on the closeable javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this useful because while I was refactoring, I often ended up adding close calls to several different layers which quickly got confusing as it was never quite clear which bit was actually responsible for closing everything.

I agree that the contract of Closeable doesn't require us to do this whole exactly-once thing - I just found it helpful to try and condense the closing responsibility into one place and not duplicate it.

public final class CloseRecordingInputStream extends InputStream {

private final InputStream delegate;
private Optional<Throwable> closeCalled = Optional.empty();

public CloseRecordingInputStream(InputStream delegate) {
this.delegate = delegate;
}

public boolean isClosed() {
return closeCalled.isPresent();
}

public void assertNotClosed() {
if (closeCalled.isPresent()) {
Assertions.fail("Expected CloseRecordingInputStream to be open but was closed", closeCalled.get());
}
}

@Override
public int read() throws IOException {
assertNotClosed();
return delegate.read();
}

@Override
public int read(byte[] bytes) throws IOException {
assertNotClosed();
return delegate.read(bytes);
}

@Override
public int read(byte[] bytes, int off, int len) throws IOException {
assertNotClosed();
return delegate.read(bytes, off, len);
}

@Override
public int available() throws IOException {
assertNotClosed();
return delegate.available();
}

@Override
public void reset() throws IOException {
assertNotClosed();
delegate.reset();
}

@Override
public void mark(int readlimit) {
assertNotClosed();
delegate.mark(readlimit);
}

@Override
public long skip(long num) throws IOException {
assertNotClosed();
return delegate.skip(num);
}

@Override
public void close() throws IOException {
closeCalled = Optional.of(new SafeRuntimeException("close was called here"));
Copy link
Contributor

Choose a reason for hiding this comment

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

should we record only the first close invocation? subsequent close calls will replace that value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, not sure it's worth the faff tbh - people can unwind one at at time :)

delegate.close();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* (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.dialogue;

import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.errorprone.annotations.CheckReturnValue;
import com.palantir.logsafe.exceptions.SafeRuntimeException;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.util.Optional;
import javax.ws.rs.core.HttpHeaders;

public final class TestResponse implements Response {

private final CloseRecordingInputStream inputStream =
new CloseRecordingInputStream(new ByteArrayInputStream(new byte[] {}));

private Optional<Throwable> closeCalled = Optional.empty();
private int code = 0;
private ListMultimap<String, String> headers = ImmutableListMultimap.of();

@Override
public CloseRecordingInputStream body() {
return inputStream;
}

@Override
public int code() {
return code;
}

@CheckReturnValue
public TestResponse code(int value) {
this.code = value;
return this;
}

@Override
public ListMultimap<String, String> headers() {
return headers;
}

@Override
public void close() {
checkNotClosed();
try {
closeCalled = Optional.of(new SafeRuntimeException("Close called here"));
inputStream.close();
} catch (IOException e) {
throw new SafeRuntimeException("Failed to close", e);
}
}

public boolean isClosed() {
return closeCalled.isPresent();
}

private void checkNotClosed() {
if (closeCalled.isPresent()) {
throw new SafeRuntimeException("Please don't close twice", closeCalled.get());
iamdanfox marked this conversation as resolved.
Show resolved Hide resolved
}
}

@CheckReturnValue
public TestResponse contentType(String contentType) {
this.headers = ImmutableListMultimap.of(HttpHeaders.CONTENT_TYPE, contentType);
return this;
}
}
4 changes: 3 additions & 1 deletion dialogue-client-verifier/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ 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'

testRuntimeOnly 'org.apache.logging.log4j:log4j-slf4j-impl'
testRuntimeOnly 'org.apache.logging.log4j:log4j-core'
Expand Down
Loading