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

[Volkan Yazıcı]

On Thu, 21 Nov 2024 21:54:46 GMT, Mark Sheppard <msheppar at openjdk.org> wrote:

>> 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 ?

Applied these suggestions in e37ee840b9681aa017f8653df68c69232a6b6c83.

> 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

Agreed. That's why I documented the choice of address and port just above the line calling the ctor. The new code you suggested using variables instead of hardcoded constants should further help with this.

> 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?

That is a very good question! Judging from [the CSR](https://bugs.openjdk.org/browse/JDK-8344527), the specification changes concern:

1. `SocketImpl::connect()`  failures (the connection cannot be established or the timeout expires before the connection is established)
2. `Socket::connect(SocketAddress)` is passed an unresolved address

(1) is verified using `connectShouldCloseOnFailures()`, and (2) is verified using `connectShouldCloseOnUnresolvedAddress()`. If, at a later time, `Socket::connect()` gets modified to **not** use the `SocketImpl`, and invalidate the assumptions held in this test, associated test methods will fail, and force the author to take the CSR requirements into account, AFAICT.

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

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