On Tue, 19 Nov 2024 10:01:22 GMT, Alan Bateman <[email protected]> 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
