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

Fri Dec 20 23:47:48 UTC 2013

Hi Tristan,

Thanks for cleaning this up. I've gone ahead and pushed the revised changeset.

http://hg.openjdk.java.net/jdk9/dev/jdk/rev/eaa533e9778a

I did take the liberty of making a couple changes... first, I had to fix some 
whitespace issues (trailing whitespace on a line) that were caught by jcheck.

Second, I changed this line from ReadTimeoutTest.java:

     throw new Error("Unexpected error happen in reader:" + ie);

to be this instead:

     throw new Error("Unexpected interrupt", ie);

I had missed this in my earlier review.

The point here is to use the exception chaining mechanism, so that when the 
stack trace for the Error is printed, it includes a "caused by" segment with the 
stack trace from the original exception that had been caught. This provides 
better diagnostic information. The general rule is that any catch-and-rethrow 
should chain up the exception cause this way. The only exception (heh) to this 
rule is if including the cause would leak sensitive information to the caller. 
This situation basically never occurs in test code, though.

s'marks

On 12/19/13 8:08 PM, Tristan Yan wrote:
> 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
>>>>>> /
>>>>> /
>>>>> /
>>>
>>
>