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

[Roger Riggs]

Hi Hamlin,

Looks good,

Thanks for the updates.

On 4/9/2018 10:49 PM, Hamlin Li wrote:
>
> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>