RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v13]

[Volkan Yazıcı]

On Fri, 22 Nov 2024 21:08:11 GMT, Mark Sheppard <msheppar at openjdk.org> wrote:

>> Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improve naming in tests
>
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 59:
> 
>> 57: 
>> 58:     private static final VarHandle SOCKET_IMPL_FACTORY_HANDLE = createSocketImplFactoryHandle();
>> 59: 
> 
> private static final int DEADSERVERPORT = 0xDEAD;

Fixed in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.

> test/jdk/java/net/Socket/CloseOnFailureTest.java line 84:
> 
>> 82: 
>> 83:         private final AtomicInteger closeInvocationCounter = new AtomicInteger(0);
>> 84: 
> 
> replacing Error with Exception in the variable names allows for easier and quicker scanning of a line     
> 
>           private final Exception bindException;
> 
>          private final Exception connectException;
> 
>          private MockSocketImpl(Exception bindException, Exception connectException) {
>              this.bindException = bindException;
>              this.connectException = connectException;
>          }
> 
> and this replacement suggestion propagates down through the rest of the code
> 
>     
>  ```
>                        Throwable
>                                |
>                                | IS_A
>                                |
>                   Error            Exception
>  
> 
> avoid Error in the variable name avoid the subconscious association with an Error class

Replaced all occurrences of `error` in variable names with `exception` in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.

> test/jdk/java/net/Socket/CloseOnFailureTest.java line 140:
> 
>> 138:                 InetAddress serverAddress = InetAddress.getLoopbackAddress();
>> 139:                 int deadServerPort = 0xDEAD;
>> 140:                 new Socket(serverAddress, deadServerPort, null, 0);
> 
> new Socket(serverAddress, DEADSERVERPORT, null, 0);       // DEADSERVERPORT declared as a constant above

Fixed in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.

> test/jdk/java/net/Socket/CloseOnFailureTest.java line 162:
> 
>> 160:         MockSocketImpl socketImpl = new MockSocketImpl(null, null);
>> 161:         try (Socket socket = new Socket(socketImpl) {}) {
>> 162:             InetSocketAddress address = InetSocketAddress.createUnresolved("no.such.host", 0xBEEF);
> 
> similar to DEADSERVERPORT
> 
> specifying a hex value, albeit a joke value, it is still a valid ephemeral port, and distracts from the main invocation InetAddress.createUnresolved, so consider a constant declaration for the port
> 
> private static final int NOSUCHHOSTPORT = 0xBEEF;
> 
> then the invocation is
> InetAddress.createUnresolved("no.such.host", NOSUCHHOSTPORT);
> 
> presenting an unambiguous statement that the InetAddress not meant to be reachable !

Reused the `DEAD_SERVER_PORT` constant here in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856214817
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856215001
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856214705
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856215137