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

[Roger Riggs]

Hi Hamlin,

Great, the new busy port algorithm looks better.
The port assigned will always be one that was available and is now busy 
to prevent the registry creation.

As expected, there is a small window  between the (try-with-finally) 
close of the server socket channel
and the 2nd createReg.  But that can't be avoided.  If the port is 
re-used in that gap
the test will fail.  (And the exception handler at 77 will see an in-use 
and retry).

67: The exception being caught is already one thrown by TestLibrary.bomb 
so it would be
cleaner to just re-throw e;  or better yet, just don't bother to catch 
the exception.
That exception should cause the test to fail.

It may be personal coding style but the createReg method with a boolean 
flag to suppress
the exception is just more confusing that putting the code in-line in 
the two places it is used.

Thanks, Roger


On 4/8/2018 4:10 AM, Hamlin Li wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>