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

[Hans Boehm]

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"?
>
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.
> >
>
>