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

[Jaikiran Pai]

On Sat, 20 Jul 2024 08:21:19 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Alan's suggestion - is not instead of isn't and closed instead of already closed
>
> test/jdk/java/net/Socket/SocketBasicExceptionsTest.java line 40:
> 
>> 38:  * @run junit SocketBasicExceptionsTest
>> 39:  */
>> 40: public class SocketBasicExceptionsTest {
> 
> I would expect these exceptions are already tested by existing tests for Socket and Server, can you check? Only asking because it looks a bit unusual to create a test for a small subset of the possible exceptions thrown by these classes.

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 (isClosed(s))
-            throw new SocketException("Socket is closed");
-        if (isConnected(s))
-            throw new SocketException("already connected");
+//        if (isClosed(s))
+//            throw new SocketException("Socket is closed");
+//        if (isConnected(s))
+//            throw new SocketException("already connected");
 
         if (!(endpoint instanceof InetSocketAddress epoint))
             throw new IllegalArgumentException("Unsupported address type");
@@ -795,10 +795,10 @@ public void connect(SocketAddress endpoint, int timeout) throws IOException {
      */
     public void bind(SocketAddress bindpoint) throws IOException {
         int s = state;
-        if (isClosed(s))
-            throw new SocketException("Socket is closed");
-        if (isBound(s))
-            throw new SocketException("Already bound");
+//        if (isClosed(s))
+//            throw new SocketException("Socket is closed");
+//        if (isBound(s))
+//            throw new SocketException("Already bound");
 
         if (bindpoint != null && (!(bindpoint instanceof InetSocketAddress)))
             throw new IllegalArgumentException("Unsupported address type");



All of them continue to pass. But I think that probably doesn't mean much because even the new test that I introduced in this PR passes, because deep within the calls, these IOException do get thrown.

I do agree that it's a bit odd to be testing this in a new test class. Maybe we rename it to `BasicTests` and then in future use this test class for additional similar basic testing of these APIs? I am even open to dropping this new test if it feels odd to introduce and unnecessary.

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

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