On Sat, 20 Jul 2024 11:06:04 GMT, Alan Bateman <al...@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
