RFR of JDK-8030175: java/rmi/registry/altSecurityManager/AltSecurityManager.java fails due to timeout

[Hamlin Li]

Hi Roger,

Both rmid and rmiregistry is running in sub-process, their log have 
already been caught and printed out by JavaVM if something get wrong, so 
for timeout due to long run, there is no more we can do. So I just 
pushed the patch.

Thank you
-Hamlin

On 2017/1/6 13:15, Roger Riggs wrote:
> Hi Hamlin,
>
> Since it is intermittent, are there any diagnostics that can be added 
> to the test in case it fails again.
> if not, ok as is.
>
> Thanks, Roger
>
>
> On 1/5/2017 8:55 PM, Hamlin Li wrote:
>>
>>
>> On 2017/1/6 6:15, Roger Riggs wrote:
>>>
>>>
>>> On 1/4/2017 10:21 PM, Hamlin Li wrote:
>>>> Hi Roger,
>>>>
>>>> Thank you for reviewing, please check comments inline.
>>>>
>>>>
>>>> On 2017/1/5 4:18, Roger Riggs wrote:
>>>>> Hi Hamlin,
>>>>>
>>>>> The original issue with timeout may be due to heavily loaded 
>>>>> systems and short timeouts.
>>>>> 15 sec is not enough on an overloaded system to wait for a process 
>>>>> to start and then die.
>>>>>
>>>>> There is no indication in this issue about port-in-use; that would 
>>>>> be a different issue.
>>>> Agree, I put 2 fixes in one patch together as there is no port in 
>>>> use issue reported, but by reviewing the code, potential port in 
>>>> use issue could happen some time in the future.
>>> Best to keep 1-1 to avoid complicating the discussion and increasing 
>>> code complexity.
>> Hi Roger,
>>
>> Got it, the new webrev for timeout issue is at: 
>> http://cr.openjdk.java.net/~mli/8030175/webrev.01/
>>
>> I just created another bug 
>> https://bugs.openjdk.java.net/browse/JDK-8172314 to track the "port 
>> in use" bug, will send out separate webrev in another thread later.
>>
>> Thank you
>> -Hamlin
>>>>>
>>>>>
>>>>> Common comments to both A and B.
>>>>>
>>>>> I'll need more time to look at B; it would be cleaner to use A if 
>>>>> it can address the issue.
>>>>> The alternative is to duplicate the code in run() in the 
>>>>> TestLibrary and not modify the RegistryImpl.
>>>> I prefer B, because
>>>>   1. Although A looks cleaner but B is simulating more like 
>>>> rmiregistry.
>>>>   2. There are some other issue for example JDK-7146543, 
>>>> JDK-8030950, JDK-8038772, fix based on version B works well, but 
>>>> fix based on version A fails.
>>>>   3. Impact of RegistryImpl modification is minimal. ( May we could 
>>>> make Registry run(String args[]) private and access it in test by 
>>>> reflection? )
>>>>   4. Although it's simple to duplicate the code in run() in the 
>>>> TestLibrary, but seems it's not a good design choice. (let's call 
>>>> it version C.)
>>>>   5. For JDK-8170728, the fix will need to modify 
>>>> sun.rmi.registry.RegistryImpl anyway.
>>>>
>>>>
>>>> Thank you for detailed review comments below.
>>>> As we have several options, I will wait for your further comments 
>>>> on choice of version A/B/C, then send out new webrev, hope I only 
>>>> need to send out one version :-).
>>>>
>>>> Thank you
>>>> -Hamlin
>>>>>
>>>>>
>>>>> JavaVM:
>>>>>  - Document the new methods.
>>>>>
>>>>> Line 232: Document the exception that may be thrown from exitValue.
>>>>>
>>>>>
>>>>> RMID:
>>>>> Line 204, 222: when adding new functions to the test library 
>>>>> please add documentation for the methods.
>>>>> There are now many variations of the methods that differ only by 
>>>>> the number arguments.
>>>>> It would be better if the name included some hint as to the 
>>>>> additional functionality.
>>>>>
>>>>> typo:  "additionalOptions" -> "add*i*tionalOptions"
>>>>>
>>>>> REGISTRY:
>>>>>   - Document the new methods.
>>>>>
>>>>>   - The name should be more indicative of its function and should 
>>>>> NOT be all caps;  RMID is an acronym where the caps make sense.
>>>>>
>>>>>   - line 105: use JavaVM.waitFor(timeout) and avoid duplicating 
>>>>> code to wait for the subprocess.
>>>>>
>>>>>   - If the subprocesses are in an unknown state it would be useful 
>>>>> to print diagnostic info from the subprocess before terminating.
>>>>>     Line 106:
>>>>>
>>>>>  - Line 124:
>>>>>  - I think I would have promoted the shutdown method to JavaVM 
>>>>> instead of creating a new cleanup method
>>>>>    to keep the code simpler.
>>>>>
>>>>>    ** The cleanup method never calls super.cleanup() so the 
>>>>> process is never destroyed!.
>>>>>
>>>>> AltSecurityManager.java:
>>>>>
>>>>>  - Line 61: the empty constructor can be removed entirely.
>>>>>
>>>>>  - Line 80: change the message to say the exception did not occur.
>>>>>     As written it implies it may have occurred but was not caught.
>>>>>
>>>>>  - Line 86: typo "a unexpected" -> "an unexpected"
>>>>>
>>>>> - Line 90: remove the printStackTrace; it is not useful and is a 
>>>>> red flag in .jtr files
>>>>>
>>>>>  - Line 125: I don't see that cleanup is better than destroy; If 
>>>>> there are doubts about destroy
>>>>>    then destroy should be fixed not avoided.
>>>>>
>>>>> Roger
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 12/26/2016 3:51 AM, Hamlin Li wrote:
>>>>>> Would you please review the below patches?
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8030175
>>>>>> webrev (version A): 
>>>>>> http://cr.openjdk.java.net/~mli/8030175/webrev.00/
>>>>>> webrev (version B): 
>>>>>> http://cr.openjdk.java.net/~mli/8030175/webrev.sun.rmi.registry.RegistryImpl.00/ 
>>>>>>
>>>>>>
>>>>>> There are 2 versions to be reviewed.
>>>>>>
>>>>>> In version A, just use RegistryRunner to replace rmiregistry.
>>>>>> In version B, refactor sun.rmi.registry.RegistryImpl to improve 
>>>>>> the testability of RMI code, and create/use RMIRegistryRunner to 
>>>>>> simulate rmiregistry.
>>>>>>
>>>>>> Thank you
>>>>>> -Hamlin
>>>>>
>>>>
>>>
>>
>