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

iamdanfox · 2020-03-26T23:42:56Z

Before this PR

Dialogue's binary handling is totally broken right now, which obviously blocks usage in internal replacement for atlas.

fixes #569

After this PR

==COMMIT_MSG==
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.
==COMMIT_MSG==

TODO:

I think we need to hook up some way to close the Response object automatically for these dudes
how defensive is this if people define their own encodings

Possible downsides?

…nary-response

changelog-app · 2020-03-27T04:07:38Z

Generate changelog in `changelog/@unreleased`

Type

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.

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2020-03-27T13:08:21Z

dialogue-client-test-lib/src/main/java/com/palantir/dialogue/CloseRecordingInputStream.java

+import java.util.Optional;
+import org.assertj.core.api.Assertions;
+
+/** A test-only inputstream which can only be closed once. */


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

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.

carterkozak · 2020-03-27T13:08:50Z

dialogue-client-test-lib/src/main/java/com/palantir/dialogue/CloseRecordingInputStream.java

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


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

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

dialogue-client-test-lib/src/main/java/com/palantir/dialogue/TestResponse.java

dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java

carterkozak · 2020-03-27T13:15:56Z

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java

+
+        ResponseClosingInputStream(InputStream inputStream, Closeable response) {
+            this.inputStream = inputStream;
+            this.response = response;


Not sure it's necessary to pass along the response object here -- all our implementations only require the body stream to be closed, and allow Response.close as a convenience to close the body.

Yeah so this was something I wasn't quite sure about - given that people are free to implement their own Response, it seemed like there's a possibility that closing the Response and closing the InputStream could release different resources somehow...

Thoughts on updating the Response javadoc to note that at least one of response or response.body must be closed, but both aren't required? That way we force implementors to handle resource release -- all our current implementations only hold resources in the body, so I don't think there's risk, and it keeps the api a touch cleaner.

Have added a bit of extra wording - does this capture your intentions?

👍 yep, thanks.

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java

…lantir/dialogue into dfox/repro-gzip-binary-response

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java

This reverts commit bb33dfc.

This reverts commit 78a4edf.

carterkozak · 2020-03-27T16:01:08Z

dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java

+    private void setBinaryGzipResponse(String stringToCompress) {
+        undertowHandler = exchange -> {
+            exchange.getResponseHeaders().put(Headers.CONTENT_TYPE, "application/octet-stream");
+            exchange.getResponseHeaders().put(Headers.CONTENT_ENCODING, "gzip");


We might want to assert that there's a Accept-Encoding request header that contains gzip to prevent confusion in the future.

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java

carterkozak

Thanks @iamdanfox!

iamdanfox · 2020-03-27T16:18:17Z

dialogue-client-verifier/src/test/java/com/palantir/verification/BinaryReturnTypeTest.java

+                .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));


maybe it's just my mac, but this takes like 28 seconds for 3GB sometimes

That’s odd considering we’re using plain text http, how long does the build system take?

svc-autorelease · 2020-03-27T16:33:48Z

Released 1.2.2

iamdanfox · 2020-03-27T16:33:51Z

Power merging because I have green CI but Circle is queuing me really hard:

https://circleci.com/workflow-run/0a2af8e1-6e2a-49c9-9f48-6b930622bae5

BinaryReturnTypeTest

37d1945

probot-autolabeler bot added the autorelease label Mar 26, 2020

iamdanfox added the no changelog label Mar 26, 2020

iamdanfox added 16 commits March 27, 2020 00:40

Try turning off ContentDecodingChannel entirely

8d5565d

make the test green

d24f8ce

Merge remote-tracking branch 'origin/develop' into dfox/repro-gzip-bi…

1788dee

…nary-response

CloseableInputStream proves ConjureBodySerde is broken

b758135

Show exactly where the close was called

4ba8567

ConjureBodySerde should not be responsible for closing

a10d2f0

Factor out TestResponse

37bffd1

DEFAULT_ENCODINGS

d249e5c

better TestResponse and CloseRecordingInputStream

e772029

test binary & optional<binary> responses

443aa53

Renames

a11d0f9

Minor fix

f24f5d3

ErrorDecoder closes response & inputstream

9ee901f

Update javadoc

8ac1c0b

format & checkstyle

79ac9d1

Close out unknowns too

4c98652

iamdanfox force-pushed the dfox/repro-gzip-binary-response branch from 38fa7e4 to 4c98652 Compare March 27, 2020 02:48

iamdanfox added 3 commits March 27, 2020 02:55

Extra safety net, just in case

1f43c4d

Move useful types to :test-lib

4a5c246

Smaller diff

55aaab6

iamdanfox force-pushed the dfox/repro-gzip-binary-response branch from d96c0d6 to 55aaab6 Compare March 27, 2020 03:05

Restore ContentDecodingChannel

2e7f992

iamdanfox changed the title ~~[test-only] Repro gzip binary return type struggles~~ Return unclosed InputStreams for binary and optional<binary> endpoints Mar 27, 2020

iamdanfox added 4 commits March 27, 2020 03:38

Closing binary InputStreams triggers Response to close

78a4edf

accept revapi

bb33dfc

rename

cd15fa7

Add generated changelog entries

2c3ff00

iamdanfox removed the no changelog label Mar 27, 2020

Move two tests

4f9431e

iamdanfox force-pushed the dfox/repro-gzip-binary-response branch from 99509e9 to 4f9431e Compare March 27, 2020 04:08

carterkozak reviewed Mar 27, 2020

View reviewed changes

iamdanfox added 5 commits March 27, 2020 14:37

Delete mockwebserver stuff

1533fa7

Move defensiveness to ConjureBodySerde

cc6dd89

Merge branch 'dfox/repro-gzip-binary-response' of ssh://github.com/pa…

ba557cd

…lantir/dialogue into dfox/repro-gzip-binary-response

Also sanity check the blocking interface

6711e94

Minimize diff

39295ca

carterkozak reviewed Mar 27, 2020

View reviewed changes

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/Encoding.java Outdated Show resolved Hide resolved

iamdanfox added 6 commits March 27, 2020 15:44

stream 3GB

3391f57

Revert "accept revapi"

138f0e1

This reverts commit bb33dfc.

Revert "Closing binary InputStreams triggers Response to close"

68dc8ed

This reverts commit 78a4edf.

Update test messages

0048ab3

format

88a4cbf

log line

3bcdfc6

carterkozak reviewed Mar 27, 2020

View reviewed changes

Extra javadoc

98d64db

carterkozak reviewed Mar 27, 2020

View reviewed changes

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java Show resolved Hide resolved

More defensive about broken encodings

8a24c6f

carterkozak approved these changes Mar 27, 2020

View reviewed changes

iamdanfox commented Mar 27, 2020

View reviewed changes

iamdanfox added the merge when ready label Mar 27, 2020

sanity check accept-encoding header in tests

399eef3

iamdanfox force-pushed the dfox/repro-gzip-binary-response branch from 9766414 to 399eef3 Compare March 27, 2020 16:23

iamdanfox merged commit 3c0b779 into develop Mar 27, 2020

iamdanfox deleted the dfox/repro-gzip-binary-response branch April 27, 2020 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iamdanfox commented Mar 26, 2020 •

edited

Loading

changelog-app bot commented Mar 27, 2020 •

edited

Loading

carterkozak Mar 27, 2020

iamdanfox Mar 27, 2020

carterkozak Mar 27, 2020

iamdanfox Mar 27, 2020

carterkozak Mar 27, 2020

iamdanfox Mar 27, 2020

carterkozak Mar 27, 2020

iamdanfox Mar 27, 2020

carterkozak Mar 27, 2020

carterkozak Mar 27, 2020

carterkozak left a comment

iamdanfox Mar 27, 2020

carterkozak Mar 27, 2020

svc-autorelease commented Mar 27, 2020

iamdanfox commented Mar 27, 2020

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

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

Conversation

iamdanfox commented Mar 26, 2020 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Mar 27, 2020 • edited Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-autorelease commented Mar 27, 2020

iamdanfox commented Mar 27, 2020

iamdanfox commented Mar 26, 2020 •

edited

Loading

changelog-app bot commented Mar 27, 2020 •

edited

Loading

Generate changelog in `changelog/@unreleased`