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
