RFR (s): 4354680: Runtime.runFinalization() silently clears interrupted flag in the calling thread

[Stuart Marks]

On 10/29/14 7:22 PM, David Holmes wrote:
> Hi Stuart,
>
> You're a brave man! :)

:-)

> Right - there is no change in behaviour with your change other than fixing the
> problem with the cleared interrupt bit. That's the best you can do in my opinion.

OK, after thinking about it, I was starting to lean that direction too.

> The spec of this method is weak enough:
>
> "When control returns from the method call, the virtual machine has made a best
> effort to complete all outstanding finalizations."
>
> that nobody should have any expectations about what a single invocation of it
> should achieve. At best they might argue that the VMs "best effort" was pretty
> poor, but I can live with that - plus it would be a distinct RFE not part of
> this bug fix.

OK.

> The fact a method is called that interacts with the interruption mechanism is
> purely an internal implementation detail. It should not bubble up to the
> specification of these methods. I don't see a need to document this aspect given
> the weakness of the spec as referenced above.

Fair enough. Less work. :-)

> That said it may be worth doing a search for all tests that use runFinalization
> to see how it is used. If used in a loop (which I suspect is common) and the
> test is timing out and so the thread is interrupted by the test harness, it will
> become a spin loop.

Yes, I've started to look at some of the uses of runFinalization(). Mostly in 
tests so far, but I haven't looked everywhere yet. Most uses seem to call some 
combination of System.gc() and System.runFinalization(), and then sleep for a 
while, so there is implicitly some notion of waiting for finalization to occur 
asynchronously. Or, they have the finalizer set some piece of state that's 
waited for.

If such a test were to time out (and interrupt the thread) during a call to 
runFinalization(), the change would preserve the interrupt bit instead of 
clearing it. But most such tests have code like,

     try { Thread.sleep(DELAY); } catch (InterruptedException ignore) { }

so the interrupt would get ignored anyway....

(Yet Another Cleanup Task: fix all the tests that ignore InterruptedException.)

> Aside, I noticed in this code:
>
>   146         forkSecondaryFinalizer(new Runnable() {
>   147             private volatile boolean running;
>   148             public void run() {
>   149                 if (running)
>   150                     return;
>   151                 final JavaLangAccess jla = SharedSecrets.getJavaLangAccess();
>   152                 running = true;
>
> that as we create a new Runnable on every call, the "running" field serves
> absolutely no purpose.

Ha. Good catch. This was probably refactored from a static field of a containing 
class. The intent was probably to prevent multiple secondary finalizer threads 
from running, but it wouldn't do this very well even if it were static.

I'll clean this up since it clearly serves no purpose as it stands.

Thanks,

s'marks