-
Notifications
You must be signed in to change notification settings - Fork 16
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
Changes from 37 commits
37d1945
8d5565d
d24f8ce
1788dee
b758135
4ba8567
a10d2f0
37bffd1
d249e5c
e772029
443aa53
a11d0f9
f24f5d3
9ee901f
8ac1c0b
79ac9d1
4c98652
1f43c4d
4a5c246
55aaab6
2e7f992
78a4edf
bb33dfc
cd15fa7
2c3ff00
4f9431e
1533fa7
cc6dd89
ba557cd
6711e94
39295ca
3391f57
138f0e1
68dc8ed
0048ab3
88a4cbf
3bcdfc6
98d64db
8a24c6f
399eef3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. */ | ||
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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.