RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed
Can I please get a review of this change which updates the specification of a few methods in `java.net.Socket` and `java.net.ServerSocket` classes to specify the `IOException` that 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. - Commit messages: - 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed Changes: https://git.openjdk.org/jdk/pull/20268/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336815 Stats: 147 lines in 3 files changed: 141 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/20268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20268/head:pull/20268 PR: https://git.openjdk.org/jdk/pull/20268
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed
On Sat, 20 Jul 2024 07:51:59 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which updates the specification of a
> few methods in `java.net.Socket` and `java.net.ServerSocket` classes to
> specify the `IOException` that 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.
src/java.base/share/classes/java/net/ServerSocket.java line 521:
> 519: *
> 520: * @throws IOException if an I/O error occurs when waiting for a
> 521: * connection, or if the socket isn't bound or is
> already closed.
Can you check the existing text as I think it uses "not bound" rather than
"isn't bound".
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 test 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685319926
PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685319724
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed
On Sat, 20 Jul 2024 08:22:20 GMT, Alan Bateman wrote: >> Can I please get a review of this change which updates the specification of >> a few methods in `java.net.Socket` and `java.net.ServerSocket` classes to >> specify the `IOException` that 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. > > src/java.base/share/classes/java/net/ServerSocket.java line 521: > >> 519: * >> 520: * @throws IOException if an I/O error occurs when waiting for >> a >> 521: * connection, or if the socket isn't bound or is >> already closed. > > Can you check the existing text as I think it uses "not bound" rather than > "isn't bound". Also "already close" might be a bit inconsistent with the existing API docs too, can you check as I think we use "is closed" in most places. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685320246
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v2]
> Can I please get a review of this change which updates the specification of a > few methods in `java.net.Socket` and `java.net.ServerSocket` classes to > specify the `IOException` that 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. 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 - Changes: - all: https://git.openjdk.org/jdk/pull/20268/files - new: https://git.openjdk.org/jdk/pull/20268/files/6cee1c9d..8990787f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20268/head:pull/20268 PR: https://git.openjdk.org/jdk/pull/20268
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v2]
On Sat, 20 Jul 2024 08:24:33 GMT, Alan Bateman wrote: >> src/java.base/share/classes/java/net/ServerSocket.java line 521: >> >>> 519: * >>> 520: * @throws IOException if an I/O error occurs when waiting >>> for a >>> 521: * connection, or if the socket isn't bound or is >>> already closed. >> >> Can you check the existing text as I think it uses "not bound" rather than >> "isn't bound". > > 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. Hello Alan, I've now updated the PR to address this text. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685356152
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v2]
On Sat, 20 Jul 2024 08:21:19 GMT, Alan Bateman 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
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v2]
On Sat, 20 Jul 2024 10:11:43 GMT, Jaikiran Pai 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: 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685380771
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v2]
On Sat, 20 Jul 2024 10:05:00 GMT, Jaikiran Pai wrote: >> 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. > > Hello Alan, I've now updated the PR to address this text. Thanks, I can review the CSR when it's ready. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685382205
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]
> Can I please get a review of this change which updates the specification of a > few methods in `java.net.Socket` and `java.net.ServerSocket` classes to > specify the `IOException` that 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. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: update specification on few other methods to match the implementation and also update test to verify the expectations - Changes: - all: https://git.openjdk.org/jdk/pull/20268/files - new: https://git.openjdk.org/jdk/pull/20268/files/8990787f..957fa7ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=01-02 Stats: 623 lines in 5 files changed: 426 ins; 142 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/20268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20268/head:pull/20268 PR: https://git.openjdk.org/jdk/pull/20268
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]
On Sat, 20 Jul 2024 11:06:04 GMT, Alan Bateman 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
Re: RFR: 8336039: Doccheck: HTML warnings, broken links and missing files in java.base documentation [v4]
On Fri, 19 Jul 2024 14:20:48 GMT, Nizar Benalla wrote:
>> Can I get a review for this change that fixes some broken links in javadoc
>> comments? The new docs are hosted
>> [here](https://cr.openjdk.org/~nbenalla/GeneratedDocs/8336039-warnings-links/).
>>
>> It's mostly fixing some relative links.
>> If using `{@docroot}` isn't ideal I can change it.
>>
>> Here is the result of running `diff -r docs-master docs` on the docs from
>> master vs and after these changes
>>
>>
>> diff -r
>> docs-master/api/java.base/java/lang/classfile/components/CodeStackTracker.html
>> docs/api/java.base/java/lang/classfile/components/CodeStackTracker.html
>> 106c106
>> <
>> ---
>>>
>> diff -r docs-master/api/java.base/java/lang/classfile/package-summary.html
>> docs/api/java.base/java/lang/classfile/package-summary.html
>> 99c99
>> <
>> ---
>>>
>> 106c106
>> <
>> ---
>>>
>> 618c618
>> <
>> ---
>>>
>> 755c755
>> <
>> ---
>>>
>> 783c783
>> <
>> ---
>>>
>> diff -r docs-master/api/java.base/java/lang/foreign/Arena.html
>> docs/api/java.base/java/lang/foreign/Arena.html
>> 142c142
>> < the segments allocated by it) becomes > href="../../../java/lang/ref/package.html#reachability">unreachable,
>> ---
>>> the segments allocated by it) becomes >> href="../../../java/lang/ref/package-summary.html#reachability">unreachable,
>> diff -r docs-master/api/java.base/java/lang/foreign/MemorySegment.Scope.html
>> docs/api/java.base/java/lang/foreign/MemorySegment.Scope.html
>> 120c120
>> < as long as it is > href="../../../java/lang/ref/package.html#reachability">reachable.
>> ---
>>> as long as it is >> href="../../../java/lang/ref/package-summary.html#reachability">reachable.
>> diff -r docs-master/api/java.base/java/lang/foreign/MemorySegment.html
>> docs/api/java.base/java/lang/foreign/MemorySegment.html
>> 1420c1420
>> < kept > href="../../../java/lang/ref/package.html#reachability">reachable
>> ---
>>> kept >> href="../../../java/lang/ref/package-summary.html#reachability">reachable
>> 1833c1833
>> < > href="../../../java/lang/ref/package.html#reachability">unreachable.
>> ---
>>> >> href="../../../java/lang/ref/package-summary.html#reachability">unreachable.
>> 1899c1899
>> < > href="../../../java/lang/ref/package.html#reachability">unreachable.
>> ---
>>> >> href="../../../java/lang/ref/package-summary.html#reachability">unreachable.
>> diff -r docs-master/api/java.base/java/lang/foreign/SymbolLookup.html
>> docs/api/java.base/java/lang/foreign/SymbolLookup.html
>> 395c395
>> ...
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>
> forgot this one
>
> Co-authored-by: Chen Liang
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
1200:
> 1198: * Construction captures the current thread's {@linkplain
> ScopedValue scoped
> 1199: * value} bindings for inheritance by threads started in the
> task scope. The
> 1200: * {@linkplain StructuredTaskScope##TreeStructure Tree
> Structure} section in the class description
You can drop the changes to STS if you want as we have an update coming that
replaces these paragraphs/links.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20251#discussion_r1685465093
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]
On Sat, 20 Jul 2024 14:44:53 GMT, Jaikiran Pai wrote: > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685475102
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]
On Sat, 20 Jul 2024 15:45:56 GMT, Alan Bateman wrote: >> 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? > >> 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. > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685476673
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
On Sat, 20 Jul 2024 15:52:08 GMT, Alan Bateman wrote: >>> 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. > >> 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685496380
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
> Can I please get a review of this change which updates the specification of a > few methods in `java.net.Socket` and `java.net.ServerSocket` classes to > specify the `IOException` that 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. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - reduce usage of "if" in the throws clause - simplify the test - Changes: - all: https://git.openjdk.org/jdk/pull/20268/files - new: https://git.openjdk.org/jdk/pull/20268/files/957fa7ec..079c5f36 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20268&range=02-03 Stats: 459 lines in 4 files changed: 126 ins; 252 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/20268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20268/head:pull/20268 PR: https://git.openjdk.org/jdk/pull/20268
Integrated: 8329398: Links in InetAddress class description show "#format"
On Fri, 19 Jul 2024 11:49:54 GMT, Aleksei Efimov wrote: > This PR modifies links in the `InetAddress` class-level javadoc showed as > `"#format"`. > In addition, it changes one grammatical error in the adjacent paragraph. This pull request has now been integrated. Changeset: b21cb44e Author:Aleksei Efimov URL: https://git.openjdk.org/jdk/commit/b21cb44e38ee8ea75e3a1c51e3a28388056a492d Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod 8329398: Links in InetAddress class description show "#format" Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/20252
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
On Sat, 20 Jul 2024 16:45:35 GMT, Jaikiran Pai wrote: >>> 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. > > 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. > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685498213
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
On Sat, 20 Jul 2024 11:10:43 GMT, Alan Bateman wrote: >> Hello Alan, I've now updated the PR to address this text. > > Thanks, I can review the CSR when it's ready. Updated docs changes look good, thanks for taking the feedback through the iterations. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685659246
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
On Sat, 20 Jul 2024 16:49:09 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the specification of >> a few methods in `java.net.Socket` and `java.net.ServerSocket` classes to >> specify the `IOException` that 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. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - reduce usage of "if" in the throws clause > - simplify the test The spec updates and test look okay to me. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20268#pullrequestreview-2190251757
Re: RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v4]
On Sat, 20 Jul 2024 16:48:54 GMT, Jaikiran Pai wrote: >> 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. > >> 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685660996
