RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use

[Hamlin Li]

Hi Roger,

I have changed to not use RegistryVM at all, please review the patch: 
http://cr.openjdk.java.net/~mli/8188897/webrev.02/

Thank you

-Hamlin


On 04/04/2018 10:15 PM, Roger Riggs wrote:
> Hi Hamlin,
>
> Reexport.java:
>
> I think this change to use a separate process for the 2nd registry 
> changes the test so that it does not
> address the original test case.  The original problem was the 
> incorrect retention of an object in
> the object table when the create of a registry in the same process 
> failed.
> Finally being able to create the registry in the same process assured 
> that the object was not
> retained in the object table.
>
> Go back to creating the 2nd registry in the test process.
>
>
> RegistryVM.java: 2, the copyright update should be  "2017, 2018," range.
>
> (I'm  really not a fan of all the RegistryVM methods with the same 
> name and minimal and implicit
> differences in their functions.  When reading the test, you have to go 
> and read the RegistryVM method
> to see what it does.  I would have preferred that the full 
> createRegistryVM (out, err, options, port) method
> was used directly in the tests.  In the case of a method used once, it 
> is an inconvenience method, not a convenience).
>
> The new terminate() method is quite similar to the existing cleanup() 
> method which does not wait.
> It would be a good cleanup to figure out if another method is really 
> needed.
> The override is going to change the behavior of the existing uses of 
> terminate().
> It should be checked that it does not break any existing uses.
>
> 178: 187: The method comments are not consistent, one says forcibly 
> and the other gracefully
> but both call requestExit and both call destroy().
>
> Thanks, Roger
>
>
> On 4/3/2018 11:35 PM, Hamlin Li wrote:
>> Hi Joe, Roger,
>>
>> Thank you for reviewing, I have refactored the test more and fix as 
>> you suggested.
>>
>> please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/
>>
>> Thank you
>>
>> -Hamlin
>>
>>
>> On 04/04/2018 10:42 AM, Joseph D. Darcy wrote:
>>> Hello,
>>>
>>> Some general comments on the coding for tests like this:
>>>
>>> * It is preferable to avoid sleep in tests to avoid increasing the 
>>> minimum amount of time a test takes to run. This helps limit the 
>>> overall time it takes the test suite to run.
>>>
>>> * If timeouts are used, it is recommend to factor the maximum time 
>>> waited with the jtreg timeout scaling factor; I don't recall its 
>>> exact name. In other words, many of our tests are run on heavily 
>>> loaded systems and the tests take longer than run than on typical 
>>> laptops and workstations so jtreg is invoked with an timeout scaling 
>>> factor. Individual tests should be sensitive to this scaling factor 
>>> for any internal timeout that need to be used.
>>>
>>> HTH,
>>>
>>> -Joe
>>>
>>> On 4/3/2018 7:48 AM, Roger Riggs wrote:
>>>> Hi Hamlin,
>>>>
>>>> Instead of a simple time delay, it would be useful to wait for the 
>>>> RegistryVM to terminate.
>>>> In killRegistry: 149,  adding subreg.waitFor() should be sufficient.
>>>>
>>>> 58: If using a 'for' loop it would be easier to understand if it 
>>>> included the usual start, increment and termination.
>>>> Instead of burying it in the exception handler.
>>>>
>>>> 59, 102, 104: the introduction of the kill boolean makes the test 
>>>> harder to understand and seems to be unnecessary.
>>>>  the killRegistry() method already will only kill the subprocess if 
>>>> it still is alive.
>>>>
>>>> Roger
>>>>
>>>> On 4/2/2018 6:33 AM, Hamlin Li wrote:
>>>>> would you please review the following patch?
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8188897
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/
>>>>>
>>>>>
>>>>> Thank you
>>>>>
>>>>> -Hamlin
>>>>>
>>>>
>>>
>>
>