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

Handle unsuccessful responses in JavaNetClient #2117

Merged
merged 2 commits into from Sep 27, 2018

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 25, 2018

HttpUrlConnection returns the body in getErrorStream rather than getInputStream when the response code is unsuccessful. Well, sometimes. Sometimes it returns null.

  1. Try to read the body from getInputStream
  2. If that throws, try to read an error from getErrorStream, if it's there.
  3. Otherwise, return an empty body.
  4. Close whatever we can find to close on dispose.

Fixes #2114.

@rossabaker rossabaker added this to the 0.18.19 milestone Sep 25, 2018
@@ -101,7 +103,12 @@ sealed abstract class JavaNetClient private (
_ <- F.delay(conn.setInstanceFollowRedirects(false))
_ <- F.delay(conn.setDoInput(true))
resp <- blocking(fetchResponse(req, conn), blockingExecutionContext)
} yield DisposableResponse(resp, F.delay(conn.getInputStream.close()))
} yield {
val dispose = F.delay(conn.getInputStream().close()).recoverWith {

This comment has been minimized.

@rossabaker

rossabaker Sep 25, 2018
Author Member

We can't just close the OutputStream here, because that throws if we didn't send a body.

We could do a disconnect of last resort, but that would terminate a persistent connection. I think what we've done here is good enough.

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Sep 25, 2018

These new tests reliably deadlock Travis. They pass locally. I'm still unclear where they're deadlocking Travis, but all the new unsafeRunSync`ing can't help.

@rossabaker rossabaker force-pushed the rossabaker:issue-2114 branch from a55e213 to f483c57 Sep 27, 2018
@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Sep 27, 2018

I think this was the cause of our intermittent client failures that have plagued us for, like, ever.

@rossabaker rossabaker mentioned this pull request Sep 27, 2018
@aeons aeons merged commit fa4c55f into http4s:release-0.18.x Sep 27, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.