RFR of 8194284: CheckRegisterInLog.java fails with java.lang.RuntimeException: CheckRegisterInLog got exception timeout 6480000ms out of range

[Hamlin Li]

On 19/01/2018 3:09 PM, David Holmes wrote:
> Hi Hamlin,
>
> On 19/01/2018 4:28 PM, Hamlin Li wrote:
>>
>> On 19/01/2018 2:11 PM, David Holmes wrote:
>>> Hi Hamlin,
>>>
>>> On 19/01/2018 3:04 PM, Hamlin Li wrote:
>>>> Hi Roger, David
>>>>
>>>> Please check the updated webrev at: 
>>>> http://cr.openjdk.java.net/~mli/8194284/webrev.00/
>>>
>>> RMID.java:
>>>
>>> This comment no longer makes sense:
>>>
>>>       // when restart rmid, it may take more time than usual because of
>>>       // "port in use" by a possible interloper (check JDK-8168975),
>>>       // so need to set a longer timeout for restart.
>>> !     private static final long RESTART_TIMEOUT = 
>>> (long)(TIMEOUT_BASE * 0.9);
>>>
>>> Actually I'm not sure it originally made sense - longer than what? 
>>> But as it stands RESTART_TIMEOUT is smaller than TIMEOUT_BASE so the 
>>> comment really seems odd. Perhaps 8168975 will shed some light on 
>>> the intent. ??
>> Hi David,
>>
>> The comment still makes sense.
>> Before 8168975, restart() calls start() which is indeed 
>> start(STARTTIME_MS), where STARTTIME_MS is 15_000L, but we found out 
>> in some situation restart() needs more time than start();
>> So in 8168975, restart() calls start(restartTimeout), where 
>> restartTimeout is RESTART_TIMEOUT in 8194284.
>
> Okay, then I suggest changing:
>
>  // so need to set a longer timeout for restart.
>
> to
>
>  // so need to set a longer timeout than STARTTIME_MS for restart.
Hi David,

Yes, it's more clear, will push the code with your suggestion.

Thank you
-Hamlin
>
> Thanks,
> David
>
>>>
>>> The TestLibrary change looks good.
>> Do you also think the comment makes sense with my explanation?
>>
>> Thank you
>> -Hamlin
>>>
>>> Thanks,
>>> David
>>>
>>>> Thank you
>>>>
>>>> -Hamlin
>>>>
>>>>
>>>> On 18/01/2018 10:33 PM, Roger Riggs wrote:
>>>>> Hi Hamlin,
>>>>>
>>>>> The base bug is that the timeoutFactor is being applied twice, 
>>>>> once in the RMID constructor
>>>>> and again in computeDeadline.  It is better to cleanup the 
>>>>> implementation of the test library
>>>>> than to fudge the values.  Either respect the timeouts passed in 
>>>>> (remove the scaling from computeDeadline)
>>>>> or consistently leave it to the lower level routines. Pick 1.
>>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>> On 1/18/2018 1:59 AM, Hamlin Li wrote:
>>>>>> Would you please review the following patch?
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8194284
>>>>>>
>>>>>> webrev as below.
>>>>>>
>>>>>>
>>>>>> Thank you
>>>>>>
>>>>>> -Hamlin
>>>>>>
>>>>>> ------------------------------------------------------------------------ 
>>>>>>
>>>>>>
>>>>>> diff -r 0dec8c41170c test/jdk/java/rmi/testlibrary/TestLibrary.java
>>>>>> --- a/test/jdk/java/rmi/testlibrary/TestLibrary.java Wed Jan 17 
>>>>>> 20:07:50 2018 -0800
>>>>>> +++ b/test/jdk/java/rmi/testlibrary/TestLibrary.java Thu Jan 18 
>>>>>> 14:54:50 2018 +0800
>>>>>> @@ -188,8 +188,12 @@
>>>>>>      public static long computeDeadline(long timestamp, long 
>>>>>> timeout) {
>>>>>>          final long MAX_TIMEOUT_MS = 3_600_000L;
>>>>>>
>>>>>> -        if (timeout < 0L || timeout > MAX_TIMEOUT_MS) {
>>>>>> +        if (timeout < 0L) {
>>>>>>              throw new IllegalArgumentException("timeout " + 
>>>>>> timeout + "ms out of range");
>>>>>> +        } else if (timeout > MAX_TIMEOUT_MS) {
>>>>>> +            System.out.format("timeout value(%d) exceeds 
>>>>>> MAX_TIMEOUT_MS(%d), "
>>>>>> +                    + "use MAX_TIMEOUT_MS instead!%n", timeout, 
>>>>>> MAX_TIMEOUT_MS);
>>>>>> +            timeout = MAX_TIMEOUT_MS;
>>>>>>          }
>>>>>>
>>>>>>          return timestamp + (long)(timeout * getTimeoutFactor());
>>>>>>
>>>>>
>>>>
>>