On Tue, 19 Nov 2024 17:21:10 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> src/java.base/share/classes/java/net/Socket.java line 706:
>>
>>> 704: try {
>>> 705: getImpl().connect(epoint, timeout);
>>> 706: } catch (IOException error) {
>>
>> At L680 we reject an endpoint that is not an InetSocketAddresss. I think we
>> should follow this with a check to ensure that the address is resolved, that
>> way Socket does all the validation before delegation to the SocketImpl. It
>> means if getImpl().connect(..) throws any exception or error then you can
>> close the Socket, meaning you can catch Throwable.
>
> The spec is still not right: we throw SocketException if the socket is
> already connected, but we do not close the socket in that case.
By comparison SocketChannel throws
`java.nio.channels.AlreadyConnectedException` which is a subclass of
IllegalSstateException (a runtime exception) and leaves the socket open. I am
not sure we could change `Socket::connect` to throw a different exception than
`SocketException` in that case due to compatibility reasons. So we now have to
choose between either:
- modify the spec to explicitely say that the socket remains open if a
SocketException is thrown because the socket is already connected
- modify the behavior to also close the socket in that case
Whatever alternative we choose should be also listed in the CSR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848779024
