On Wed, 20 Nov 2024 08:23:29 GMT, Alan Bateman <al...@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
