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 retrying of SocketTimeoutExceptions in HttpConnector #9008

Conversation

artem-zinnatullin
Copy link
Contributor

As part of investigation of #8974 I found that recent change 982e0b8 broke retries of SocketTimeoutException in HttpConnector, intention of that change was good but lack of tests resulted in broken logic.

IntelliJ highlights the problem — branch with instanceof SocketTimeoutException would have never been executed:

Screen Shot 2019-07-29 at 3 26 11 PM


This PR adds missing tests and fixes the logic to still present SocketTimeoutException as IOException for upstream consumers while handling it properly internally in the HttpConnector.

@irengrig irengrig added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged and removed untriaged labels Jul 31, 2019
@jin jin requested a review from aehlig July 31, 2019 17:28
@ghost ghost mentioned this pull request Aug 3, 2019
Copy link
Contributor

@aehlig aehlig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@bazel-io bazel-io closed this in 338829f Aug 5, 2019
katre pushed a commit that referenced this pull request Aug 6, 2019
As part of investigation of #8974 I found that recent change 982e0b8 broke retries of `SocketTimeoutException` in `HttpConnector`, intention of that change was good but lack of tests resulted in broken logic.

IntelliJ highlights the problem ? branch with `instanceof SocketTimeoutException` would have never been executed:

<img width="764" alt="Screen Shot 2019-07-29 at 3 26 11 PM" src="https://user-images.githubusercontent.com/967132/62089179-d7bfa400-b21c-11e9-882a-1c1fe1fcb683.png">

---

This PR adds missing tests and fixes the logic to still present `SocketTimeoutException` as `IOException` for upstream consumers while handling it properly internally in the `HttpConnector`.

Closes #9008.

PiperOrigin-RevId: 261675244
katre pushed a commit that referenced this pull request Aug 7, 2019
As part of investigation of #8974 I found that recent change 982e0b8 broke retries of `SocketTimeoutException` in `HttpConnector`, intention of that change was good but lack of tests resulted in broken logic.

IntelliJ highlights the problem ? branch with `instanceof SocketTimeoutException` would have never been executed:

<img width="764" alt="Screen Shot 2019-07-29 at 3 26 11 PM" src="https://user-images.githubusercontent.com/967132/62089179-d7bfa400-b21c-11e9-882a-1c1fe1fcb683.png">

---

This PR adds missing tests and fixes the logic to still present `SocketTimeoutException` as `IOException` for upstream consumers while handling it properly internally in the `HttpConnector`.

Closes #9008.

PiperOrigin-RevId: 261675244
katre pushed a commit that referenced this pull request Aug 12, 2019
As part of investigation of #8974 I found that recent change 982e0b8 broke retries of `SocketTimeoutException` in `HttpConnector`, intention of that change was good but lack of tests resulted in broken logic.

IntelliJ highlights the problem ? branch with `instanceof SocketTimeoutException` would have never been executed:

<img width="764" alt="Screen Shot 2019-07-29 at 3 26 11 PM" src="https://user-images.githubusercontent.com/967132/62089179-d7bfa400-b21c-11e9-882a-1c1fe1fcb683.png">

---

This PR adds missing tests and fixes the logic to still present `SocketTimeoutException` as `IOException` for upstream consumers while handling it properly internally in the `HttpConnector`.

Closes #9008.

PiperOrigin-RevId: 261675244
@philwo
Copy link
Member

philwo commented Sep 13, 2019

Hi @artem-zinnatullin,

the test you added in this PR seems to be flaky at time, for example see here:

https://buildkite.com/bazel/bazel-bazel/builds/9795#175f3020-00fd-4163-bfc6-716f7eb94662
https://buildkite.com/bazel/bazel-bazel/builds/9791#15545e80-f124-422d-9c51-09c681118506
https://buildkite.com/bazel/bazel-bazel/builds/9790#6c4ed173-9a61-4342-81a7-1e76b8268df6

You can find the logs for the failed test in the "Artifacts" tab. The log always looks like this and it only seems to happen on Windows:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //src/test/java/com/google/devtools/build/lib/bazel/repository/downloader:DownloaderTestSuite
-----------------------------------------------------------------------------
JUnit4 Test Runner
..................E..............................................................................
Time: 28.809
There was 1 failure:
1) socketTimeout_retries(com.google.devtools.build.lib.bazel.repository.downloader.HttpConnectorTest)
expected: 1
but was : 2
	at com.google.devtools.build.lib.bazel.repository.downloader.HttpConnectorTest.socketTimeout_retries(HttpConnectorTest.java:323)

FAILURES!!!
Tests run: 96,  Failures: 1


BazelTestRunner exiting with a return value of 1
JVM shutdown hooks (if any) will run now.
The JVM will exit once they complete.

-- JVM shutdown starting at 2019-09-13 11:47:54 --

Do you have an idea what could cause this? I will deactivate the test temporarily while we're figuring this out.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@philwo philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants