8336815: Several methods in java.net.Socket and ServerSocket do not specify behavior when already bound, connected or closed #20268
Conversation
…ify behavior when already bound, connected or closed
|
/csr needed |
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
|
@jaikiran This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 43 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
rfr
|
@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request. @jaikiran please create a CSR request for issue JDK-8336815 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
| * throw the specified exceptions | ||
| * @run junit SocketBasicExceptionsTest | ||
| */ | ||
| public class SocketBasicExceptionsTest { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
IOExceptionwhen 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks, and doesn't surprise me that it's not explicitly specified in other methods. I think the spec change looks okay although I think it would be better to not repeat the "if" in each of the thrown, e.g. "if the bind operation fails, the socket is already bound, or the socket is closed". Up to you if you want to stick with what you have.
Reading the updates, some of the socket options have "an error in the underlying protocol, such as a TCP error" but none of these methods are doing I/O, they are just setting or reading socket options. They should say "if there is an error SO_RCVBUF" or whatever. It's not important here, it's just jumping out when reading these old descriptions.
You asked about specifying the more specific SocketException. These APIs are pluggable via SocketImpl so specifying a more specific exception could potentially invalidate existing implementations. I don't know if there are any other implementations in 2024 but I think changing anything here would require further thought on the compatibility impact.
There was a problem hiding this comment.
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.
Thanks. My only comment is that using a MethodSource and SocketOp/ServerSocketOp in these tests looks a bit overkill. You may find it a bit simpler to just using assertThrow, something like assertThrows(SocketException.class, s::getReceiveBufferSize). I don't have a strong opinion, I just prefer simpler tests I think.
There was a problem hiding this comment.
I've updated the PR to remove the usages of the parameterized test and instead inline the operations within the test itself. The tests continue to pass.
There was a problem hiding this comment.
I think it would be better to not repeat the "if" in each of the thrown, e.g. "if the bind operation fails, the socket is already bound, or the socket is closed".
Done - I've removed the repeated "if" from the throws clause.
some of the socket options have "an error in the underlying protocol, such as a TCP error" but none of these methods are doing I/O, they are just setting or reading socket options. They should say "if there is an error SO_RCVBUF" or whatever. It's not important here, it's just jumping out when reading these old descriptions.
I decided to leave them as-is in this PR, given how many of those there are. I think we can decide to clean them up in a separate PR if needed.
You asked about specifying the more specific SocketException. These APIs are pluggable via SocketImpl so specifying a more specific exception could potentially invalidate existing implementations. I don't know if there are any other implementations in 2024 but I think changing anything here would require further thought on the compatibility impact.
Given that context, leaving them in their current form sounds reasonable to me.
There was a problem hiding this comment.
The updated tests looks okay.
No action required but just to say that future work could expand the test for when socket is bound before closed or connected before closed, thus ensure the methods have the specified behavior for all possible states. For the method that don't throw then it would be possible to assert the return values.
| @@ -518,7 +518,7 @@ public SocketAddress getLocalSocketAddress() { | |||
| * client socket implementation factory}, if one has been set. | |||
| * | |||
| * @throws IOException if an I/O error occurs when waiting for a | |||
| * connection. | |||
| * connection, or if the socket isn't bound or is already closed. | |||
There was a problem hiding this comment.
Can you check the existing text as I think it uses "not bound" rather than "isn't bound".
There was a problem hiding this comment.
Also "already closed" might be a bit inconsistent with the existing API docs too, can you check as I think we use "is closed" in most places.
There was a problem hiding this comment.
Hello Alan, I've now updated the PR to address this text.
There was a problem hiding this comment.
Thanks, I can review the CSR when it's ready.
There was a problem hiding this comment.
Updated docs changes look good, thanks for taking the feedback through the iterations.
… and also update test to verify the expectations
jaikiran
changed the title
|
Thank you Alan for the review and inputs on the spec and the test. The CSR is now ready for review https://bugs.openjdk.org/browse/JDK-8336868. |
openjdk
bot
added
ready
|
The CSR has been approved. Joe suggested a tiny change to add a note that many of these methods throw an exception when invoked on a closed socket. I've updated this PR to include that note. No other changes. |
| @@ -736,6 +736,9 @@ private void ensureCompatible(SocketImpl si) throws IOException { | |||
| * <p> If this socket has an associated channel then the channel is closed | |||
| * as well. | |||
| * | |||
| * <p>Once closed, several of the methods defined by this class will throw | |||
There was a problem hiding this comment.
The existing paragraphs use "
", probably want to keep it consistent to avoid mixing styes in the one method.
| * use (i.e. can't be reconnected or rebound). A new socket needs to be | ||
| * created. | ||
| * use (i.e. can't be reconnected or rebound) and several of the methods defined | ||
| * by this class will throw an exception when invoked on the closed socket. A new |
There was a problem hiding this comment.
I think "if invoked" might be better than "when invoked" here
There was a problem hiding this comment.
Fixed and updated the PR to address both these places.
|
/integrate |
|
Going to push as commit 04e8cb8.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Can I please get a review of this change which updates the specification of a few methods in
java.net.Socketandjava.net.ServerSocketclasses to specify theIOExceptionthat the implementation currently already throws?This is merely a doc update and doesn't change the implementation.
Given that these exceptions can now be asserted, a new jtreg test has been introduced to verify the exceptions thrown from these APIs. The test passes. tier testing is currently in progress. I'll open a CSR shortly.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20268/head:pull/20268$ git checkout pull/20268Update a local copy of the PR:
$ git checkout pull/20268$ git pull https://git.openjdk.org/jdk.git pull/20268/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20268View PR using the GUI difftool:
$ git pr show -t 20268Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20268.diff
Webrev
Link to Webrev Comment