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

[Hamlin Li]

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
>>>
>>
>