On Sat, 20 Jul 2024 08:21:19 GMT, Alan Bateman <al...@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
