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

[Alan Bateman]

On Mon, 25 Nov 2024 09:28:35 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 with a new target base due to a merge or a rebase. The pull request now contains 19 commits:
> 
>  - Merge remote-tracking branch 'upstream/master' into Socket-CloseOnFailure-8343791
>  - Reflect behavioral changes to `SocketAdaptor`
>  - Improve variable names
>  - Improve naming in tests
>  - Handle `SocketTimeoutException` in `NioSocketImpl::connect()`
>  - Revert `UHE` message change in `NioSocketImpl`
>  - Remove self-reference guard in `closeSuppressingExceptions()`
>  - Add back incorrectly removed `SocketTimeoutException` Javadoc
>  - Remove unused `port` variable
>  - Replace `UnknownHostException exception` with `var uhe`
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/9576546b...423a78c3

I see Mark is reviewing the test so I didn't put much time into looking at the test. The main thing is for this test is that it exercises Socket.connect and uses Socket::isClosed to test if the Socket is closed or not after the connect fails. I think we need to test at least the following:

Socket unbound and unconnected, Socket connect fails, Socket should be closed
Socket bound and unconnected, Socket connect fails, Socket should be closed
Socket connected, Socket connect fails, Socket should not be closed
Socket unbound and unconnected, Socket connect to unresolved address, Socket should be closed
Socket bound and unconnected, Socket connect to unresolved address, Socket should be closed
Socket connected, Socket connect connect to unresolved address Socket should not be closed
You shouldn't need a mock SocketImpl. I assume you have this because the test is testing the Socket constructors mostly.

For the socket adapter then you could have the tests be parametrised so they are called with an newly created Socket (unbound/unconnected), that way you can test new Socket() and SocketChannel.open().socket().

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

PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2497489169