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
8308336: Test java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java failed: java.net.BindException: Address already in use #14177
Conversation
👋 Welcome back DarraghClarke! A progress list of the required criteria for merging this PR into |
@DarraghClarke The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
@@ -73,7 +73,7 @@ public void startServerSocket() throws Exception { | |||
|
|||
control.serverSocket = new ServerSocket(); | |||
control.serverSocket.setReuseAddress(true); | |||
control.serverSocket.bind(new InetSocketAddress("127.0.0.1", 54321)); | |||
control.serverSocket.bind(new InetSocketAddress("127.0.0.1", 0)); |
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.
While you're at it can you change the address to use InetAddress.getLoopbackAddress()
here, and the URIBuilder in createConnection() below?
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.
based on Daniel's last comment and requested change then I think it would be useful to add two additional run requests to the test which set the network properties java.net.preferIPv4Stack=true and java.net.preferIPv6Addresses=true, for completeness.
You never know what might "pop out of the woodwork"
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.
I added those changes and verified that the tests are still passing
private HttpURLConnection createConnection() throws Exception { | ||
URL url = URIBuilder.newBuilder() | ||
.scheme("http") | ||
.host(InetAddress.getLoopbackAddress()) |
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.
This will change the call flow of the test in a slightly subtle way. In the original the "host" is supplied i.e. localhost which should map to the loopback address, and in the change the loopback IP address is being supplied diectly. In terms of equivalence then supplying a host string might be more appropriate:
.host("localhost")
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.
No that's precisely what we want to avoid. Because how "localhost" maps to an InetAddress depends on the machine configuration, which is a recipe for intermittent failures.
I'd suggest:
URL url = URIBuilder.newBuilder()
.scheme("http")
.localhost()
(which is actually the same)
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.
At a glance there isn't a .localhost()
only a .loopback()
is that what you were referring to?
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.
Yes! Sorry - my mistake.
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.
I pushed that change now and am currently rerunning tests
test/jdk/java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java
Outdated
Show resolved
Hide resolved
…tinueTest.java Co-authored-by: Daniel Fuchs <67001856+dfuch@users.noreply.github.com>
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.
@DarraghClarke 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 258 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Spent some time double checking tests and everything seems to be stable |
/integrate |
@DarraghClarke |
/sponsor |
Going to push as commit a48bcf3.
Your commit was automatically rebased without conflicts. |
@dfuch @DarraghClarke Pushed as commit a48bcf3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
HttpURLConnectionExpectContinueTest
was throwing an error due to port being hardcoded, updated test to let the system decide which port to use.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14177/head:pull/14177
$ git checkout pull/14177
Update a local copy of the PR:
$ git checkout pull/14177
$ git pull https://git.openjdk.org/jdk.git pull/14177/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14177
View PR using the GUI difftool:
$ git pr show -t 14177
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14177.diff
Webrev
Link to Webrev Comment