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

[David Holmes]

Hi Stuart,

You're a brave man! :)

On 30/10/2014 11:04 AM, Stuart Marks wrote:
> Hi all,
>
> Please review this small change that fixes this bug in
> System.runFinalization() and Runtime.runFinalization().
>
> Bug:     https://bugs.openjdk.java.net/browse/JDK-4354680
>
> Webrev:  http://cr.openjdk.java.net/~smarks/reviews/4354680/webrev.0/
>
> This is probably the simplest approach, which is to reassert the
> interrupt bit if an InterruptedException is caught.
>
> I'm slightly concerned by this, though, since if the join() was
> interrupted (either because an interrupt was received while waiting to
> join the thread, or the caller's interrupt bit was set at the time
> join() was called), the method will return immediately, without waiting
> for pending objects to be finalized. This seems to violate the spirit
> (if not the letter) of this method's specification.
>
> This is the way runFinalization() has always behaved, though. The only
> difference with this change is that the interrupt bit is restored
> instead of being cleared.

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.

> One could imagine an alternative approach which would loop until the
> secondary finalizer thread actually completes. However, it's hard for me
> to imagine any code relying on runFinalization() synchronously clearing
> the finalization queue. Since objects become finalizable asynchronously,
> how would the caller know that the "right" set of objects had been
> finalized? It seems to me that this isn't very useful, it's a change in
> behavior, and it's harder to test.

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.

> If we don't wait for the secondary finalizer thread to complete, perhaps
> it would be appropriate to add a note to the doc about the early return
> in case of an interrupt. Perhaps something like,
>
>      If the calling thread is interrupted before or during this call,
>      this call may return before all outstanding finalizations have been
>      completed, and the calling thread's interrupt status will be set.
>
> Would something like this be appropriate? Probably this would be added
> to both System.runFinalization() and Runtime.runFinalization().

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.

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.

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.

Cheers,
David

> s'marks