RFR: 8336815: Socket.bind/connect and ServerSocket.bind/accept do not specify behavior when already bound, connected or closed [v3]

[Alan Bateman]

On Sat, 20 Jul 2024 15:45:56 GMT, Alan Bateman <alanb at 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