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

[Volkan Yazıcı]

On Wed, 20 Nov 2024 08:23:29 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Apply review feedback from Alan
>
> src/java.base/share/classes/java/net/Socket.java line 625:
> 
>> 623:      *          is already connected or the socket is closed
>> 624:      * @throws  UnknownHostException if the endpoint is an unresolved
>> 625:      *          {@link InetSocketAddress}, the socket is closed
> 
> It might be better to drop ", the socket is closed" from the exception description as it may be mis-read to mean that it gets thrown when the socket is closed.

Fixed in 424fc8ccd3a495a49f859f0cfc563cf463b3efeb.

> src/java.base/share/classes/java/net/Socket.java line 662:
> 
>> 660:      * </ol>
>> 661:      * <p> Besides above noted cases, the socket will also be closed when a
>> 662:      * connection attempt fails with an {@link IOException}.
> 
> Is this a left-over, I assume it should be removed.

Doh! :facepalm: Fixed in 424fc8ccd3a495a49f859f0cfc563cf463b3efeb.

> src/java.base/share/classes/java/net/Socket.java line 669:
> 
>> 667:      *          is already connected or the socket is closed
>> 668:      * @throws  SocketTimeoutException if timeout expires before connecting,
>> 669:      *          the socket is closed
> 
> The updated method description makes it clear that the the socket will be closed if the timeout expires before the connection is established, shouldn't need to change the exception description.

Fixed in 424fc8ccd3a495a49f859f0cfc563cf463b3efeb.

> src/java.base/share/classes/java/net/Socket.java line 709:
> 
>> 707:         }
>> 708: 
>> 709:         try {
> 
> At (old) L680, it checks that endpoint is an InetSocketAddress. We should also check that it resolved, as in:
> 
>         if (epoint.isUnresolved()) {
>             close();
>             throw new UnknownHostException(epoint.getHostName() + " is unresolved");
>         }
> 
> The benefit is the the constructor does the validation before is delegates to the SocketImpl. We could change the SocketImpl to throw IAE for this case, it doesn't matter because it will never happen if Socket rejects it.

Implemented in 424fc8ccd3a495a49f859f0cfc563cf463b3efeb, along with a test. Are you sure we want to have a `close()` there instead of `closeQuietly()`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850254216
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850254539
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850254974
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850251862