On Sat, 20 Jul 2024 15:45:56 GMT, Alan Bateman <al...@openjdk.org> 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
