RFR for JDK-7168267: TEST_BUG: Cleanup of rmi regression tests (activation and others)

[Tristan Yan]

Thanks Stuart
I changed ReadTimeoutTest.java only apply CountdownLatch part. Please 
review.

http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.02/

Thank you
Tristan

On 12/20/2013 10:47 AM, Stuart Marks wrote:
> Hi Tristan,
>
> Changes mostly look good.
>
> There is an ignored InterruptedException in both versions of 
> UseCustomSocketFactory.java, but that was there already; it's not 
> clear what should be done in this case anyway. This is something to 
> keep in mind for a future cleanup. Hm, some duplicate code here as 
> well, again something to think about for the future.
>
> There is a serious problem with the change to ReadTimeoutTest.java, 
> however. The change removes the (old) line 72, which is
>      TestIface stub = impl.export();
> probably because there was an IDE warning that the variable "stub" is 
> unused. This much is true, but it's essential for impl.export() to be 
> called, because that exports the object, which creates a socket using 
> the socket factory, which eventually results in the fac.whichPort() 
> call below returning the port that was open. In the absence of the 
> export() call, whichPort() returns zero which causes the test to abort 
> immediately.
>
> In addition, the refactoring to use try-with-resources changes the 
> order of execution of certain code, and it changes the scope of things 
> handled by the finally-block.
>
> One purpose of the finally-block is to unexport the remote object so 
> it makes sense to begin the try-block immediately following the 
> export. The original code did this (well, only after a benign local 
> variable declaration). The change moves the try-block few lines down, 
> which means there is a larger window of time within which the 
> finally-block won't be executed. This isn't obviously a problem, but 
> it's a change nonetheless.
>
> Also, the change alters the order of opening the client socket and the 
> "connecting to listening port" message, so the message comes after the 
> port is opened, instead of before. Again, an apparently small change, 
> but if there's an exception opening the port, the port number being 
> opened won't be printed out.
>
> The main point of the changes to this file, however, is good, which is 
> to replace the unsafe use of multi-thread access to a boolean array 
> and polling of that value, with a CountDownLatch. So that part of the 
> change should go in. The problem is the apparently innocuous code 
> cleanups (use of try-with-resources, removal of apparently unused 
> local variable) which cause the test to break or otherwise change its 
> behavior.
>
> I could go ahead and push this changeset, omitting the changes to 
> ReadTimeoutTest.java. Or, you could update the changeset to revert all 
> of the changes to ReadTimeoutTest.java except for those necessary to 
> implement the use of CountDownLatch. Either way is fine with me.
>
> Which would you prefer?
>
> s'marks
>
>
> On 12/18/13 6:51 AM, Tristan Yan wrote:
>> Hi Everyone
>> Please review the code fix for bug JDK-7168267
>>
>> http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.01/ 
>> <http://cr.openjdk.java.net/%7Etyan/JDK-7168267/webrev.01/>
>>
>> This is a cleanup for RMI tests. trying to use real timeout to 
>> replace a fixed number of loop.
>> Thank you
>>
>> Tristan
>>
>>
>> On 12/12/2013 05:33 AM, Stuart Marks wrote:
>>> On 12/10/13 6:10 PM, Tristan Yan wrote:
>>>> /Hi everyone
>>>> I am working on bug JDK-7168267
>>>>
>>>
>>> Correct link is
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-7168267
>>>
>>>> Root Cause:
>>>> - Per Stuart's comment, this is a clean up bug.
>>>>
>>>> Suggested Fix:
>>>> - Will use timeout to replace loop.
>>>
>>> We should probably look at specific cases for this. There are places 
>>> where the test is waiting for some external service to become ready 
>>> (e.g., rmiregistry). There's no notification for things like this so 
>>> wait-with-timeout cannot be used. Pretty much the only thing that 
>>> can be done is to poll reasonably often until the service is ready, 
>>> or until the timeout is exceeded.
>>>
>>>> - Also I am fixing two test's performance
>>>> java/rmi/activation/Activatable/forceLogSnapshot - method 
>>>> waitAllStarted is
>>>> using sleep to poll 50 restartedObject to be true, we can use modern
>>>> CountDownLatch to implement blocking-time wait.
>>>> java/rmi/activation/Activatable/checkAnnotations - We can subclass
>>>> ByteArrayOutputStream which support notification when data was 
>>>> written. Also use
>>>> two thread wait output string and error string to be not null.
>>>
>>> These sound reasonble. Go ahead and file sub-tasks for these and 
>>> then choose one to work on first. (I think it will get too confusing 
>>> if we try to work on them all simultaneously.) Either post a 
>>> detailed description of what you intend to do, or if it's simple 
>>> enough, just post a webrev.
>>>
>>> s'marks
>>>
>>>>
>>>> Please let me know if you have any comments or suggestions.
>>>> / /
>>>> Thank you
>>>> Tristan
>>>>
>>>> On 12/05/2013 09:02 AM, Stuart Marks wrote:
>>>> /
>>>>> /On 12/3/13 11:05 PM, Tristan Yan wrote:
>>>>> /
>>>>>> /I am working on 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-7168267. This bug is
>>>>>> asking performance improvement for RMI test. Because this would 
>>>>>> involve
>>>>>> different RMI tests. I’d like to use this cr as an umbrella bug, 
>>>>>> create sub-cr
>>>>>> for different test. Then I can make progress on sub-cr. Please 
>>>>>> let me know your
>>>>>> opinion on this.
>>>>>> /
>>>>> /
>>>>> Actually JDK-7168267 is more about various test cleanups, and 
>>>>> JDK-8005436 is
>>>>> more about performance. Both bugs, though, make general statements 
>>>>> about "the
>>>>> RMI tests" and don't have much information about specific actions 
>>>>> that need to
>>>>> be taken. I've added some notes to JDK-7168267 about some cleanups 
>>>>> that could
>>>>> be done.
>>>>> / /
>>>>> If there are specific actions for either of these bugs, then yes, 
>>>>> creating
>>>>> Sub-Tasks of these bugs and fixing them individually is the right 
>>>>> thing to do.
>>>>> / /
>>>>> s'marks
>>>>> /
>>>> /
>>>> /
>>
>