Review Request for CR : 7144861 RMI activation tests are too slow

[Olivier Lagneau]

Hi Stuarts, David,

I agree with your arguments/points discussed below.
I understand the argument against swallowing or only reasserting the 
interrupt when catching IE.

I have gone again quickly through all the rmi tests and don't think this 
code takes into account interuption,
or intend to manage properly cancellation/termination. Cumulating all 
the tests case, there is even between
1 and 2 min spent in Thread.sleep() call that are not managed properly 
(IE mostly swallowed).
So IE is not correctly handled in [almost all] these rmi tests.

The tests can in addition be run with -samevm option of jtreg, and it is 
also certainly meaningfull to manage
correctly IE in that case too.


> On 5/7/12 10:32 PM, David Holmes wrote:
> The clean solution would assume  interrupts were used to signify 
> cancellation and code accordingly,
> but doing that consistently all through may be a larger cleanup than 
> Olivier wants to try now. 
Given that we want to push the code quickly, I don't think I should go 
for such a large IE cleanup for this fix,
which is meant to provide better exceution speed only.

I suggest to follow Stuart's proposal first (i.e. reassert and return 
immediately in my code changes) ,
and create a dedicated new CR regarding proper handling of IE in all the 
rmi tests (low priority).
Do you agree with this ?

Thanks,
Olivier.

David Holmes said  on date 5/9/2012 5:53 AM:
> On 9/05/2012 12:53 PM, Stuart Marks wrote:
>> On 5/7/12 10:32 PM, David Holmes wrote:
>>> As a general principle library code that catches IE and doesn't
>>> rethrow it
>>> should re-assert the interrupt state.
>>>
>>> But this isn't library code it is a stand-alone test framework as I
>>> understand
>>> it. So if the framework doesn't use interruption at all then the
>>> response is
>>> somewhat arbitrary. As you point out this code is all over the place 
>>> with
>>> regard to IE handling at present. The clean solution would assume
>>> interrupts
>>> were used to signify cancellation and code accordingly, but doing that
>>> consistently all through may be a larger cleanup than Olivier wants to
>>> try now.
>>
>> This is indeed test code, but just as library code should be decoupled
>> from the caller, test code should be decoupled from its framework. So I
>> think the general principle applies.
>>
>> I'm not asking that the all code be audited to make sure handles IE
>> properly everywhere. But Olivier's changes did touch some code that
>> handles IE, which naturally raises the question of the proper way to
>> handle IE. Reasserting the interrupt bit and continuing around the loop
>> just causes it to spin until the loop is exhausted, which doesn't seem
>> sensible.
>
> Right - that certainly has to be changed.
>
>> Returning early seems sensible. Applying the rule says that
>> the code should reassert the interrupt bit before returning.
>
> If interruptible code is looping around an interruptible method that 
> must be re-executed then the usual approach is to re-assert the 
> interrupt outside the loop eg:
>
> boolean wasInterrupted = false;
> while (cond) {
>   try {
>       interruptibleOp();
>   }
>   catch (InterruptedException ie) {
>     wasInterrupted = true;
>     continue;
>   }
> }
> if (wasInterrupted)
>   Thread.currentThread().interrupt();
>
> In this case however this seems more like a fail-fast situation so 
> immediate return seems in order - though the fact the test was 
> interrupted does have to be reported to the test harness somehow.
>
> David
> -----
>
>> An alternative would be to have the code rethrow or propagate IE, but
>> this requires additional refactoring and probably adding throws clauses
>> to methods, so it's probably not warranted.
>>
>> My recommendation to Olivier is to run through the files in the
>> changeset and modify the catch blocks as follows:
>>
>> catch (InterruptedException ie) {
>> Thread.currentThread().interrupt();
>> // maybe print a log message
>> return; // or break, as appropriate
>> }
>>
>> s'marks