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

[Mark Sheppard]

On Thu, 21 Nov 2024 10:47:54 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:
> 
>   Handle `SocketTimeoutException` in `NioSocketImpl::connect()`

test/jdk/java/net/Socket/CloseOnFailureTest.java line 139:

> 137:                 // They just need to be _valid enough_ to reach to the point where both `SocketImpl#bind()` and `SocketImpl#connect()` are invoked.
> 138:                 // Failure will be triggered by the injected `SocketImpl`.
> 139:                 new Socket(address, 0xDEAD, address, 0xBEEF);

Using new Socket(address, 0xDEAD, address, 0xBEEF);       using hex as port number, a bit too clever!  

I think a constant would be preferable for us simple folk.

private static final int DEADSERVERPORT = 0xDEAD; // or  57005

I think it is preferable for the local port to use 0 i.e. an OS ephemeral allocation, as it is moot in this test context

One observation I’d make on this is that, when scanning the code and not carrying the details of the MockSocketImpl with you, one is inclined to think that this will intermittently lead to BindException and ConnectException as both 0xDEAD and 0xBEEF are in the ephemeral port range

Thus, a general question on the scope of testing. When considering the use of mock object in the call flows. Are these tests sufficient for the testing that the spec changes are upheld by the full default implementation ?

test/jdk/java/net/Socket/CloseOnFailureTest.java line 210:

> 208:         private static final String ERROR_MESSAGE = "intentional test failure";
> 209: 
> 210:         private static final class ForBindFailure {

ForConnectionFailure ForBindfFailure are factories abstraction
Their method are factory methods (the world is addicted to of methods) fabricating a TestCase

ofIOException
ofIllegalArgumentException
ofIOException() 

A simple change to ConnectionFailureFactory and BindFailureFactory
With methods like ioExceptionTestCase illegalArgumentException

Lead to readable code like

TestCase.ConnectionFailureFactory.ioExceptionTestCase()

TestCase.BindFaiureFactory.illegalExceptionTestCase()

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1852960421
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1852961418