On Thu, 23 Nov 2023 11:35:11 GMT, Daniel Fuchs <[email protected]> wrote:
>> RMI Connections (in general) should use a timeout on the Socket connect call
>> by default.
>>
>> JMX connections use RMI and some connection failures never terminate. The
>> hang described in 8316649 is hard to reproduce manually: the description
>> says it can be caused by a firewall that never returns a response.
>>
>> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> has other timeouts but nothing to control the initial Socket connect.
>>
>> Defaulting to a 1 minute timeout on connect has no effect on existing tests
>> for RMI and JMX, and should go unnoticed in applications unless there really
>> is a significant connection delay. Specifying zero for the new System
>> Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path
>> of a connect with no timeout.
>>
>> I have a test, but it is not realistically usable: although specifying a 1
>> millisecond timeout will often fail (as expected/desired for the test), it
>> will often pass as the connection happens immediately.
>
> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPDirectSocketFactory.java
> line 46:
>
>> 44: private static final int connectTimeout = // default 1 minute
>> 45: AccessController.doPrivileged((PrivilegedAction<Integer>) () ->
>> 46:
>> Integer.getInteger("sun.rmi.transport.tcp.initialConnectTimeout", 60 *
>> 1000).intValue());
>
> An exception here will prevent the class from being initialized, any
> subsequent attempts to use the class will then produce hard to diagnose
> error. If the class is not loaded by the main thread, such exception/error
> could be swallowed unless an uncaught exception handler was registered. I
> would suggest implementing the logic in a static block, catch any exception
> and revert to default behaviour (i.e. 0) if the value supplied for the
> property is not an integer, or not a valid integer, etc...
IIRC, the default agent uses / may use its own socket factories - are we still
coming here even with those factories?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403260460
