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

[Volkan Yazıcı]

On Tue, 19 Nov 2024 10:01:22 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 suggestions by Alan & Daniel
>
> src/java.base/share/classes/java/net/Socket.java line 500:
> 
>> 498:                 bind(localAddr);
>> 499:             connect(address);
>> 500:         } catch (IOException | IllegalArgumentException | SecurityException error) {
> 
> Naming the exception "error" is a bit strange as this isn't catching Errors. I think the only change need to the Socket constructor is to change line 500 to catch Throwable, leave the try-catch to add the suppressed exception as it's much more readable.

Fixed in 7e633d8d213a950304c550f93a1697c1f2530a05.

> src/java.base/share/classes/java/net/Socket.java line 617:
> 
>> 615:      * </ol>
>> 616:      * <p> Besides above noted cases, the socket will also be closed when a
>> 617:      * connection attempt fails with an {@link IOException}.
> 
> I think it would be better to put the new paragraph before the paragraph on interrupt. I think we'll end up with something like " If the endpoint is an unresolved InetSocketAddress, the connection cannot be established, or the timeout expires before the connection is established, then the Socket is closed and IOException is thrown".

Implemented the requested change in 7e633d8d213a950304c550f93a1697c1f2530a05. Note that I excluded the  timeout-related part for `connect(SocketAddress)`, since it doesn't receive timeout as input. Yet, cloned this changes to `connect(SocketAddress,int)` and retained the timeout-related part. Let me know if you want me to add the timeout-related part to the `connect(SocketAddress)` Javadoc too.

> src/java.base/share/classes/java/net/Socket.java line 623:
> 
>> 621:      *          is already connected or the socket is closed
>> 622:      * @throws  UnknownHostException if the endpoint address is
>> 623:      *          {@linkplain InetSocketAddress#isUnresolved() not resolved},
> 
> I think better to say "if the endpoint is an unresolved InetSocketAddress".

Fixed in 7e633d8d213a950304c550f93a1697c1f2530a05.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848995261
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848998292
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848998624