RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]

[Jaikiran Pai]

On Sat, 20 Jul 2024 11:06:04 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> I had looked around in the test/jdk/java/net/Socket and test/jdk/java/net/ServerSocket tests to see if this is already tested. But I can't see anything that does this specific testing. I now decided to do a local change to the source to not throw the `IOException` when already bound/closed and reran the tests in these directories:
>> 
>> diff --git a/src/java.base/share/classes/java/net/ServerSocket.java b/src/java.base/share/classes/java/net/ServerSocket.java
>> index e1271fc9deb..3280f9edc4f 100644
>> --- a/src/java.base/share/classes/java/net/ServerSocket.java
>> +++ b/src/java.base/share/classes/java/net/ServerSocket.java
>> @@ -366,10 +366,10 @@ public void bind(SocketAddress endpoint) throws IOException {
>>       * @since 1.4
>>       */
>>      public void bind(SocketAddress endpoint, int backlog) throws IOException {
>> -        if (isClosed())
>> -            throw new SocketException("Socket is closed");
>> -        if (isBound())
>> -            throw new SocketException("Already bound");
>> +//        if (isClosed())
>> +//            throw new SocketException("Socket is closed");
>> +//        if (isBound())
>> +//            throw new SocketException("Already bound");
>>          if (endpoint == null)
>>              endpoint = new InetSocketAddress(0);
>>          if (!(endpoint instanceof InetSocketAddress epoint))
>> @@ -532,10 +532,10 @@ public SocketAddress getLocalSocketAddress() {
>>       * @see SecurityManager#checkAccept
>>       */
>>      public Socket accept() throws IOException {
>> -        if (isClosed())
>> -            throw new SocketException("Socket is closed");
>> -        if (!isBound())
>> -            throw new SocketException("Socket is not bound yet");
>> +//        if (isClosed())
>> +//            throw new SocketException("Socket is closed");
>> +//        if (!isBound())
>> +//            throw new SocketException("Socket is not bound yet");
>>          Socket s = new Socket((SocketImpl) null);
>>          implAccept(s);
>>          return s;
>> diff --git a/src/java.base/share/classes/java/net/Socket.java b/src/java.base/share/classes/java/net/Socket.java
>> index 1809687d5c0..cddbeb54a5a 100644
>> --- a/src/java.base/share/classes/java/net/Socket.java
>> +++ b/src/java.base/share/classes/java/net/Socket.java
>> @@ -737,10 +737,10 @@ public void connect(SocketAddress endpoint, int timeout) throws IOException {
>>              throw new IllegalArgumentException("connect: timeout can't be negative");
>>  
>>          int s = state;
>> -        if (isClo...
>
>> I had looked around in the test/jdk/java/net/Socket and test/jdk/java/net/ServerSocket tests to see if this is already tested. But I can't see anything that does this specific testing. I now decided to do a local change to the source to not throw the `IOException` when already bound/closed and reran the tests in these directories:
> 
> If there are holes in the testing then we need to fill them but I'm not sure about introducing SocketBasicExceptionsTest to test a small subset of the possible exceptions is the best approach. Also the tests for ServerSocket are in a different directory. What would you think about a ClosedSocketTest and ClosedServerServerTest (or better name) in each directory to test that the methods throw as expected, it could test that close, isXXX, ... don't throw as expected. We can do the same for bind and Socket.connect.

I've now introduced a ClosedSocketTest and a ClosedServerSocketTest in their relevant directories. Each of them invoke various operations on a closed Socket/ServerSocket instance and verify that they throw the expected exception or complete normally if expected to do so.

When writing this test, I noticed that a few other methods on ServerSocket and Socket also needed an update to their specification to match their current implementation. I have included those changes as well. Once we come to an agreement about these changes, I will update the title of the issue to make it clear that this covers more APIs than what the title currently says.

One related question is - some of these APIs, like the bind() are specified to throw an IOException when the implementation throws a SocketException (a subclass of IOException). Some other APIs within the same classes specify that they throw this exact SocketException. Should bind() and a few other APIs which currently specify IOException be updated to say SocketException to closely match their implementation?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685457901