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

[Volker Simonis]

On Fri, Oct 12, 2018 at 11:57 AM Hans Boehm <hboehm at google.com> wrote:
>
> On Thu, Oct 11, 2018 at 3:42 PM Peter B. Kessler <Peter.B.Kessler at oracle.com>
> wrote:
>
> > The JavaDoc for `Reference.reachabilityFence(Object ref)` [
> > http://hg.openjdk.java.net/jdk/jdk/file/62523934374c/src/java.base/share/classes/java/lang/ref/Reference.java#l509]
> > says
> >
> >         @param ref the reference. If {@code null}, this method has no
> > effect.
> >
> > It might be true that if the actual parameter is a constant `null` the
> > method has no effect.  I understand the intent of the method if applied to
> > a simple variable (e.g., `this`).  What is the effect of calling the method
> > with a complex expression that yields an `Object`?  How does the compiler
> > know which lifetime to extend?  When is the actual parameter evaluated?
> > What if the complex expression yields `null`?  Does the method still have
> > "no effect"?

Hi Peter,

that's an interesting question and when I first read it I was a little
confused. However, I think you can easily understand the answer Hans
gave below, if you extend the example from the API doc of
reachabilityFence as follows:

public void action() {
  Vector v = new Vector();
  v.add(this)
  try {
    // ...
    int i = myIndex;
    Resource.update(externalResourceArray[i]);
  } finally {
    Reference.reachabilityFence(v.getFirst());
  }
}

I this case, "v" will be kept alive and not "this" as in the original
code. However, the Vector "v" obviously keeps a reference to "this",
so "this" will be kept alive and reachable transitively. You can
easily extend this to more complex examples.

> >
> Peter -
>
> Logically, the call to reachabilityFence() needs to be compiled like any
> other call, with the argument evaluated before the call. The argument needs
> to look reachable to the garbage collector at the point of the call, as it
> would for a real call. But the call doesn't actually do anything; it just
> ensures that the argument is around at that point. But the compiler can't
> "know" that the call doesn't do anything and the argument is dead; it must
> be treated as live up to the call, in spite of the fact that it's not used.
> (The compiler also can't "know" much else about the call, since we don't
> want it to move the call up past other memory accesses. I gather that for
> hotspot this isn't much of an issue.)
>
> In reality, the fact the the GC only runs at certain points allows this to
> be relaxed a little.
>
> If you pass null, then the target of null needs to be reachable at that
> point. Which is vacuously true.
>
>
> > The try-with-resource solution also operates on expressions, but there it
> > is more obvious what to make a strong reference to to keep a particular
> > Object reachable.
> >
> > Hans's `@Cleaned` annotation can only be applied to fields, not
> > expressions.  Does that make it less useful?
> >
>
> The proposal currently defines it in terms of implied reachabilityFences on
> objects that are dereferenced to get to the annotated field. The simplest
> implementation is intended to be that we perform no dead reference
> elimination, i.e. keep all references around to the end of their scope, in
> methods that access an @Cleaned field. The detailed rules are there to
> allow implementations to do better.
>
> There may be corner cases in which reachabilityFence() is more useful. I'm
> not proposing to get rid of it.
>
> Hans
>
>
> >                         ... peter
> >
> > On 10/11/18 12:56 PM, Hans Boehm wrote:
> > > On Wed, Oct 10, 2018 at 10:42 PM Erik Osterlund <
> > erik.osterlund at oracle.com>
> > > wrote:
> > >>
> > >> Hi Hans,
> > >>
> > >> Would users be happier if e.g. try with resources could be relied upon
> > >> having things reachable in a scope for this pattern?
> > >>
> > >> try (SlipperyObject x = getSlipperyObject()) {
> > >>    // x is strongly reachable in this scope
> > >> }
> > >> // x won’t die until here
> > >>
> > >> Seems more intuitive for me to reason about the reachability in a scope
> > >> than manually analysing the uses of locals.
> > >>
> > >> I suppose you would have to implement AutoCloseable to be allowed to use
> > >> such a construct. But a SlipperyObject class could maybe wrap that and
> > even
> > >> call reachability fence in its close method for portability.
> > >>
> > >> The trailing close function might already have the same effect as
> > >> reachabilityFence.
> > >>
> > >> Just an idea.
> > >>
> > >> Thanks,
> > >> /Erik
> > >>
> > >
> > > I think the easiest and most common use case is one in which a class C
> > > includes a field that needs to be explicitly cleaned up when an instance
> > of
> > > C is dropped.
> > > (Unfortunately even libcore contains a bunch of more complicated
> > patterns,
> > > but let's ignore those here.)
> > >
> > > In that case, the alternatives are basically:
> > >
> > > Current:
> > >
> > > class C {
> > >    private long f; // A handle to some resource needing explicit
> > cleaning.
> > >    ...
> > >    Foo someMethod(C that) {
> > >      try {
> > >        ... access f and that.f ...
> > >      } finally {
> > >        reachabilityFence(this);
> > >        reachabilityFence(that);
> > >     }
> > >     ... repeat for other methods ...
> > > }
> > >
> > > IIUC, with your proposal, it becomes
> > >
> > > class C {
> > >    private long f; // A handle to some resource needing explicit
> > cleaning.
> > >    ...
> > >    Foo someMethod(C that) {
> > >      try (C me = this; c myThat = that) {
> > >        ... access f and that.f, or equivalently me.f and myThat.f ...
> > >     }
> > >     ... repeat for other methods ...
> > > }
> > >
> > > That's clearly shorter by a constant factor. But to me it doesn't look
> > > consistent with the normal try-with-resources statement.
> > >
> > > My alternative proposal would reduce this to:
> > >
> > > class C {
> > >    @Cleaned private long f; // A handle to some resource needing explicit
> > > cleaning.
> > >    ...
> > >    Foo someMethod(C that) {
> > >          ... access f and that.f ...
> > >    }
> > >     ... Other methods also need no special treatment ...
> > > }
> > >
> > > This has the advantage that the annotation is confined to one place,
> > > arguably where it belongs. The annotation overhead is shorter by a factor
> > > of O(<number of methods>). It doesn't get around the fact that this is
> > > really difficult to explain to a non-compiler-expert in full detail. But
> > I
> > > think it's possible to quickly explain the annotation in a way that will
> > > make sense to most people and will generally lead them to correct code.
> > >
> > > Full disclosure: This remains messy in some cases, notably if you let f
> > > escape from C by handing it to a caller. There's some argument that that
> > > should never happen for a properly designed API. It puts the caller into
> > > the position of explicitly having to reason about object lifetimes.
> > > Unfortunately things like this do seem to be hard to avoid ins some
> > cases.
> > > Even in the messy cases, so far I've almost always found @Cleaned to be
> > > more convenient than explicit reachabilityFences.
> > >
> >
> >