Finalizer being run while class still in use (escape analysis bug)

[Luke Hutchison]

On Wed, Oct 10, 2018 at 3:50 AM Andrew Haley <aph at redhat.com> wrote:

> > This makes finalizers a lot less reliable and more difficult to use
> > than they could be.
> >
> > Wouldn't it make sense to consider the invocation targets in the stack
> > during liveness analysis, so that out of the cases you gave, "finalizers
> > may be run too soon (as here,) too late, or never", at least the "too
> soon"
> > case is fixed?
>
> We considered doing that; the discussion is online somewhere. I was in
> favour of doing what you suggest, but there is some performance impact
> of preserving liveness for all methods. In the end it was decided that
> we'd provide reachabilityFence.
>

The stack already has to be walked for reachability analysis, and there
would no doubt be some performance impact for marking invocation targets as
reachable, but that already has to be done for a conceivably much larger
set of objects than just the number of stack frames, so it seems like the
overall impact to performance would be minimal. This is especially true
given that only classes that override finalize() would need to be tracked
in this way. Was this benchmarked before this decision was made, in order
to determine actual rather than theorized performance impact?

I don't know what went into the discussion you refer to, but not doing what
is arguably "the right thing" in the name of what is probably a tiny
performance margin doesn't seem strongly justifiable. I understand your
arguments about elision of methods etc., but I really can't believe that a
currently running method would not be considered a strong enough reference
to "this".

Forcing users to put reachabilityFence wrappers (or "synchronized (this)"
wrappers) around every single method of every object that contains a
finalizer seems like a very poor fix to this problem.

It is well known that a lot of things about finalizers are broken. But why
not make them a little less broken?

You need two threads to have a race condition. I'm not sure what
> you're trying to ask: the reachabilityFence must run strictly after
> the body of the try clause, and it the object will be reachable until
> then.
>

Right -- one thread is the garbage collector.

What I'm asking is with a method invocation "new A().someMethod()", is
there conceivably a gap in time between when the runtime realizes that
there are no more references to the "A" object in the caller, and when it
realizes that there is a fenced reference to "this" in "someMethod"?

I'm guessing the answer is no, otherwise the reachabilityFence mechanism
would not always work, but I was asking about the exact ordering of the
dropping of the invocation target reference, and adding object references
within the invoked method to the set of reachable objects.