-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8296610: java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java failed with "BindException: Address already in use: connect" #11634
Conversation
👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into |
Webrevs
|
System.gc(); | ||
try { | ||
Thread.sleep(500); | ||
} catch (InterruptedException iex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it only fails rarely, would it be safer to delay a few seconds to allow more time for resources to be freed? Otherwise, the change looks fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. What delay would you recommend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2-3 sec maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dfuch This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 4 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 0dce5b8.
Your commit was automatically rebased without conflicts. |
The test has been observed failing once in the CI, on windows, with a BindException (address already in use) on the client side. This usually means that the system has run out of ephemeral ports or file descriptors. The proposed fix will catch the BindException, run the gc, wait a bit, and retry once. No guarantee that this will actually work - but maybe worth a try, though I worry a bit the authentication count might get wrong if the request that gets the BindException is the first one that follows the 401/407 response - in case the connection was closed after receiving 401/407. Since we have no visibility of which call to connect() fails, it is hard to cater for this possibility without making the test too permissive.
The alternative would be to throw a SkipException in case BindException is received.
While working on the fix I noticed that some parts of the test had been commented out waiting for a RFE to be implemented. Since the RFE has been implemented since then, I went out and uncommented that part of the test.
Comments welcome.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11634/head:pull/11634
$ git checkout pull/11634
Update a local copy of the PR:
$ git checkout pull/11634
$ git pull https://git.openjdk.org/jdk pull/11634/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11634
View PR using the GUI difftool:
$ git pr show -t 11634
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11634.diff