Review Request for CR : 7144861 RMI activation tests are too slow (original) (raw)

Olivier Lagneau olivier.lagneau at oracle.com
Wed May 9 15:26:27 UTC 2012

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

More information about the core-libs-dev mailing list