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
