7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout #25690

jaikiran · 2025-06-09T07:39:02Z

Can I please get a review of this doc-only change which proposes to add a @apiNote to the Socket.connect(SocketAddress endpoint, int timeout) method? This addresses https://bugs.openjdk.org/browse/JDK-7116990.

As noted in that issue, users can find it surprising that when the Socket.connect(...) method is called with a timeout value, then if that timeout value happens to be greater than the connect timeout that operating systems typically impose, then a IOException gets thrown instead of the SocketTimeoutException. The change in this PR proposes to add a @apiNote which explains this current behaviour.

If this requires a CSR, I'll open one once we settle on the proposed text.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8359249 to be approved

Issues

JDK-7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout (Bug - P3)
JDK-8359249: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25690/head:pull/25690
$ git checkout pull/25690

Update a local copy of the PR:
$ git checkout pull/25690
$ git pull https://git.openjdk.org/jdk.git pull/25690/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25690

View PR using the GUI difftool:
$ git pr show -t 25690

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25690.diff

Using Webrev

Link to Webrev Comment

… because of TCP timeout

bridgekeeper · 2025-06-09T07:39:41Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-09T07:40:51Z

@jaikiran 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:

7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout

Reviewed-by: alanb, dfuchs

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 113 new commits pushed to the master branch:

382f870: 8357976: GenShen crash in swap_card_tables: Should be clean
8c760e7: 8359418: Test "javax/swing/text/GlyphView/bug4188841.java" failed because the phrase of text pane does not match the instructions
afa52e4: 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
... and 110 more: https://git.openjdk.org/jdk/compare/52338c94f610611a9e89a6ccbe6f2c6cd768b50a...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-06-09T07:41:17Z

@jaikiran The following label will be automatically applied to this pull request:

net

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.

mlbridge · 2025-06-09T07:44:32Z

Webrevs

dfuch · 2025-06-09T09:59:22Z

src/java.base/share/classes/java/net/Socket.java

@@ -621,6 +621,12 @@ public void connect(SocketAddress endpoint) throws IOException {
     *        {@code SocketException} with the interrupt status set.
     * </ol>
     *
+     * @apiNote Establishing a TCP/IP connection is subject to connection timeout settings
+     * in the operating system. The typical timeout is 60 seconds. If the operating system


Suggested change

* in the operating system. The typical timeout is 60 seconds. If the operating system

* in the operating system. The typical operating system timeout is 60 seconds. If the operating system

I would suggest repeating "operating system timeout" here too, to remove confusion with the simple API timeout which also appears later in this paragraph.

FWIW:
Stating a typical value of 60 seconds timeout can lead to a misconception or set an expectation ... From from TCP standards and depending on which literature you read (OS docs or unix networking socket programming) then 75 secs should be a more typical default

I think the 60 seconds comes from a perceived setting on linux. For example if a linux config of
net.ipv4.tcp_syn_retries = 6 is set and the RTO == 1 sec, with a backoff policy of doubling the timeout each retry, then the connect timeout would expect to be 63 secs

It would be better to say that, the value is OS dependent, influenced by OS network setting relating to syn receive timeouts and the number of syn retries, and governed by the TCP retransmission timer implementation, rather than stating a particular value.

Hello Mark,
Alan's thought was that it might be OK to have that sentence about the typical 60 second timeout. The primary guidance to developers here is that "The {@code timeout} specified to this method is typically a timeout value that is shorter than the operating system timeout." so that they set a lower value when appropriate.

Alan @AlanBateman, do you suggest we continue with this text or would any update be necessary?

I think it is an unnecessary quantification, is somewhat inaccurate, and set an expectation of a developer that this is gospel or axiomatic. Indicating that it is OS dependent should be sufficient.

This is all that needs to be said. There is no need to state any typical values, but if you do then those values need to be factually correct, and for the currently supported platforms 60 seconds is not typical, it's 21, 75, and 128 seconds

The proposed wording in the current draft looks okay. It explains to the reader that establishing a TCP/IP connection is subject to an operating system timeout. It gives a sense of what that timeout might be, it's not hours or days, it's tens of seconds. I don't think we should attempt to list specific timeouts for specific operating system versions and configurations.

why are you insisting on specifying 60 seconds? It does not exist on any supported OS platform. There is no need to specify any value in the apiNote. Doing so may lead to a possible misinterpretation or misunderstanding of the intended purpose.

Maybe we could say:

The typical operating system timeout ranges within tens of seconds to minutes.

👍 very good suggestion

I'll however change that text to say "connect timeout" shortly.

Done.

Maybe we could say:

The typical operating system timeout ranges within tens of seconds to minutes.

I see that Mark is OK with this text, so I've updated the PR to use this text. Alan, does this updated text look OK?

AlanBateman

Good

dfuch

Not sure this requires a CSR. It might - if only for the sake of clarifying expectations for JCK too.

jaikiran · 2025-06-11T11:48:10Z

/csr

openjdk · 2025-06-11T11:48:25Z

@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jaikiran please create a CSR request for issue JDK-7116990 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

jaikiran · 2025-06-11T17:29:37Z

The CSR is now ready for review https://bugs.openjdk.org/browse/JDK-8359249

AlanBateman · 2025-06-11T18:31:45Z

The CSR is now ready for review https://bugs.openjdk.org/browse/JDK-8359249

It's okay to have a CSR but no usually needed for API notes.

…out"

AlanBateman · 2025-06-17T07:24:37Z

src/java.base/share/classes/java/net/Socket.java

+     * seconds to minutes. If the operating system timeout expires before the
+     * {@code timeout} specified to this method then an {@code IOException} is thrown.
+     * The {@code timeout} specified to this method is typically a timeout value that is
+     * shorter than the operating system timeout.


The previous version was a bit clearer but this version is okay too. You might want to consider "is in the range of tens of seconds to ..." rather than "ranges within tens of seconds to ..".

Done, I've updated the PR.

jaikiran · 2025-06-17T08:36:43Z

The CSR is now ready for review https://bugs.openjdk.org/browse/JDK-8359249

It's okay to have a CSR but no usually needed for API notes.

Noted. For this one, I'll go ahead and finalize the already filed CSR.

jaikiran · 2025-06-18T02:05:02Z

The CSR has been approved. Thank you all for the inputs and reviews. I'll go ahead with the integration.

jaikiran · 2025-06-18T02:05:17Z

/integrate

openjdk · 2025-06-18T02:06:25Z

Going to push as commit 2f63d3a.
Since your change was applied there have been 113 commits pushed to the master branch:

382f870: 8357976: GenShen crash in swap_card_tables: Should be clean
8c760e7: 8359418: Test "javax/swing/text/GlyphView/bug4188841.java" failed because the phrase of text pane does not match the instructions
afa52e4: 8210549: Runtime.exec: in closeDescriptors(), use FD_CLOEXEC instead of close()
... and 110 more: https://git.openjdk.org/jdk/compare/52338c94f610611a9e89a6ccbe6f2c6cd768b50a...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-18T02:06:32Z

@jaikiran Pushed as commit 2f63d3a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

7116990: (spec) Socket.connect(addr,timeout) not clear if IOException…

909d7d2

… because of TCP timeout

openjdk bot added the rfr Pull request is ready for review label Jun 9, 2025

openjdk bot added the net [email protected] label Jun 9, 2025

dfuch reviewed Jun 9, 2025

View reviewed changes

Daniel's suggestion

d6978f0

AlanBateman approved these changes Jun 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2025

dfuch approved these changes Jun 9, 2025

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label Jun 11, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Jun 11, 2025

improve text

124e097

dfuch approved these changes Jun 13, 2025

View reviewed changes

jaikiran added 2 commits June 15, 2025 10:04

Daniel's suggestion for the text

138c92d

Mark's suggestion - use "connect timeout" instead of "connection time…

381919b

…out"

dfuch approved these changes Jun 16, 2025

View reviewed changes

AlanBateman reviewed Jun 17, 2025

View reviewed changes

Alan's suggestion

d78f08f

AlanBateman approved these changes Jun 17, 2025

View reviewed changes

dfuch approved these changes Jun 17, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jun 18, 2025

openjdk bot added the integrated Pull request has been integrated label Jun 18, 2025

openjdk bot closed this Jun 18, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 18, 2025

jaikiran deleted the 7116990 branch June 18, 2025 02:07

	* in the operating system. The typical timeout is 60 seconds. If the operating system
	* in the operating system. The typical operating system timeout is 60 seconds. If the operating system

7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout #25690

7116990: (spec) Socket.connect(addr,timeout) not clear if IOException because of TCP timeout #25690

Uh oh!

Conversation

jaikiran commented Jun 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

mlbridge bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msheppar Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msheppar Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Jun 11, 2025

Uh oh!

openjdk bot commented Jun 11, 2025

Uh oh!

jaikiran commented Jun 11, 2025

Uh oh!

AlanBateman commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Jun 17, 2025

Uh oh!

jaikiran commented Jun 18, 2025

Uh oh!

jaikiran commented Jun 18, 2025

Uh oh!

openjdk bot commented Jun 18, 2025

Uh oh!

openjdk bot commented Jun 18, 2025

Uh oh!

Uh oh!

jaikiran commented Jun 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 9, 2025 •

edited

Loading

mlbridge bot commented Jun 9, 2025 •

edited

Loading

msheppar Jun 9, 2025 •

edited

Loading

msheppar Jun 13, 2025 •

edited

Loading