Skip to content

Conversation

@baywet
Copy link
Member

@baywet baywet commented Sep 15, 2020

Fixes #313

Changes proposed in this pull request

Other links

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will take a look shortly.

@baywet baywet added this to the 2.2.0 milestone Sep 15, 2020
@baywet baywet self-assigned this Sep 16, 2020
MIchaelMainer
MIchaelMainer previously approved these changes Sep 17, 2020
Copy link
Collaborator

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

@baywet
Copy link
Member Author

baywet commented Sep 18, 2020

@MIchaelMainer this actually fixes 3 problems:

  • Okhttp on jre11+ initiates the connection using HTTP/2 by default instead of http/1.1 for jre8. Our services for some reason send a connection reset when they see HTTP/2 coming. -> locks to http/1.1
  • Our retry handler only retries on receiving responses (429, 504,...) but it doesn't retry on connection drop/reset etc (for some reason AGS sends a connection reset when it gets hot). -> enables retry connection from the http lib.
  • Our retry handler was never integration tested -> adds a integration test that tests heavy load working. (which also allowed me to debug the other two cases)

@baywet baywet merged commit a8edc68 into dev Sep 18, 2020
@baywet baywet deleted the bugfix/retry-connection-errors branch September 18, 2020 20:50
@yschimke
Copy link

yschimke commented Oct 30, 2020

It's worth testing this again with HTTP/2 with OkHttp 4.10.0-RC1

https://square.github.io/okhttp/changelog/#version-4100-rc1

Fix: Attempt to read the response body even if the server canceled the request. This will cause some calls to return nice error codes like HTTP/1.1 429 Too Many Requests instead of transport errors like SocketException: Connection reset and StreamResetException: stream was reset: CANCEL.

@baywet
Copy link
Member Author

baywet commented Oct 30, 2020

Hey @yschimke
Thanks for letting us know! I logged #554 as we plan to upgrade ok http with that version

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClientException thrown when tcp connection is closed

5 participants