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

[Hamlin Li]

On 10/04/2018 3:09 AM, Roger Riggs wrote:
> Hi Hamlin,
Hi Roger,

Thank you for review.
>
> 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).
Yes, you're right.
>
> 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's to catch IOException by ServerSocketChannel.open, bind. Seems it's 
a little confusing, so I declare at main function to throw IOException.
>
> 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.
I think it's mainly because of original parameter name remoteOK, so 
rename it as expectException, and move it to the last parameter.

new webrev is updated in place:
http://cr.openjdk.java.net/~mli/8188897/webrev.02/

Thank you
-Hamlin
>
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>