RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v17]

[Alan Bateman]

On Tue, 26 Nov 2024 20:57:18 GMT, Volkan Yazıcı <duke at openjdk.org> wrote:

>> This PR, addressing 8343791, changes `Socket::connect()` methods to close the `Socket` in the event that the connection cannot be established, the timeout expires before the connection is established, or the socket address is unresolved.
>> 
>> `tier3` tests pass against the 9f00f61d3b7fa42a5e23a04f80bb4bb1a2076ef2.
>
> Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Split tests into multiple files

I scanned the test update. My personal view is that there way too much here. JDK tests typically only use a small subset of the features that JUnit provides. So very surprised to see the use of extensions in these tests. For tests in this area then the priority should be on having tests that are easy to maintain and diagnose when there are test failures, e.g. think about a test failure in 2 years time where someone has to figure out test base with different subclasses for different APIs. I can imagine someone being puzzled by "Oio" in the name. My guess is that you mean "old I/O" but it looks really odd here.

Daniel may be able to give you other advice. My view is keep it simple, don't over engineer the tests, and think about diagnosability when tests fail. In this case I think we just need one test ConnectFailTest, or better name, that exercises Socket.connect with the various scenarios. Think CloseOnConnectFailureTestBase without the ceremony. The test methods can be parametized so they execute with sockets created with new Socket() and SocketChannel.open().socket().

I'm not sure what to say about the mock SocketImpl and testing the Socket constructor, maybe you can create a test only issue in JBS and contribute it that way. I'd prefer this PR to stay focused on Socket.connect.

src/java.base/share/classes/sun/nio/ch/SocketAdaptor.java line 93:

> 91:                     throw new SocketException("Socket is closed");
> 92:             if (sc.isConnected())
> 93:                     throw new SocketException("Already connected");

Would you mind changing this to use 4-space instead of 8-space indent?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2503541844
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1860436868