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

Fix deserialization of structured errors after retries are exhausted #630

Merged
merged 6 commits into from
Apr 9, 2020

Conversation

iamdanfox
Copy link
Contributor

Before this PR

All versions of dialogue contain this bug: when we exhaust retries and pass the final response to the ErrorDecoder, the response body has already been closed so it can't read the json. This means every structured error comes out as an UnknownRemoteException:

com.palantir.conjure.java.api.errors.UnknownRemoteException: Error 429. (Failed to parse response body as SerializableError.)
	at com.palantir.conjure.java.dialogue.serde.DefaultClients.newUnknownRemoteException(DefaultClients.java:120)
	at com.palantir.conjure.java.dialogue.serde.DefaultClients.block(DefaultClients.java:94)
	at com.palantir.dialogue.example.SampleServiceBlocking$1.voidToVoid(SampleServiceBlocking.java:35)
	...
Caused by: com.palantir.conjure.java.api.errors.UnknownRemoteException: Error 429. (Failed to parse response body as SerializableError.)
	at com.palantir.conjure.java.dialogue.serde.ErrorDecoder.decode(ErrorDecoder.java:57)
	at com.palantir.conjure.java.dialogue.serde.ConjureBodySerDe$EmptyBodyDeserializer.deserialize(ConjureBodySerDe.java:333)
	at com.palantir.conjure.java.dialogue.serde.ConjureBodySerDe$EmptyBodyDeserializer.deserialize(ConjureBodySerDe.java:320)
	at com.google.common.util.concurrent.AbstractTransformFuture$TransformFuture.doTransform(AbstractTransformFuture.java:242)
	...
Caused by: org.apache.http.ConnectionClosedException: Premature end of Content-Length delimited message body (expected: 164; received: 0)
	at org.apache.http.impl.io.ContentLengthInputStream.read(ContentLengthInputStream.java:178)
	at org.apache.http.conn.EofSensorInputStream.read(EofSensorInputStream.java:135)
	at java.base/sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:284)
	at java.base/sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:326)
	at java.base/sun.nio.cs.StreamDecoder.read(StreamDecoder.java:178)
	at java.base/java.io.InputStreamReader.read(InputStreamReader.java:185)
	at java.base/java.io.Reader.read(Reader.java:229)
	at com.google.common.io.CharStreams.copyReaderToBuilder(CharStreams.java:119)
	at com.google.common.io.CharStreams.toStringBuilder(CharStreams.java:177)
	at com.google.common.io.CharStreams.toString(CharStreams.java:163)
	at com.palantir.conjure.java.dialogue.serde.ErrorDecoder.toString(ErrorDecoder.java:77)
	at com.palantir.conjure.java.dialogue.serde.ErrorDecoder.decode(ErrorDecoder.java:55)

This is obviously different from c-j-r's behaviour, and means anyone's catch(RemoteException) blocks won't get triggered.

After this PR

==COMMIT_MSG==
Structured errors are now correctly deserialized when requests exhaust all available retries. (Previously they would always appear as an UnknownRemoteException.)
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Apr 8, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Structured errors are now correctly deserialized when requests exhaust all available retries. (Previously they would always appear as an UnknownRemoteException.)

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Great find!

@iamdanfox
Copy link
Contributor Author

👨‍🎨

@bulldozer-bot bulldozer-bot bot merged commit 7a210d0 into develop Apr 9, 2020
@bulldozer-bot bulldozer-bot bot deleted the dfox/retrying-channel-tidying branch April 9, 2020 00:08
@svc-autorelease
Copy link
Collaborator

Released 1.11.2

@@ -0,0 +1,6 @@
type: fix
fix:
description: Structured errors are now correctly deserialized when requests exhaust
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth I think this was only a bug for 503 and 429 responses where our infrastructure sends empty response bodies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants