RFR 9: 8165641 : Deprecate Object.finalize

[Hans Boehm]

The approach Gil is advocating certainly has its attraction. The reason I'm
not really enthusiastic, and don't really view the language design problem
as solved anymore, is that I'm no longer certain that the majority of
programmers will ever be able to use reachabilityFence correctly. The rules
are really, really subtle and unintuitive, especially for JNI-intensive
code. I don't have a termination proof for Gil's step 4.

Android's BigInt.java is a good illustration of the problem. This is a
wrapper around a native (BoringSSL) bignum package, which generally
forwards most operations directly to native code. But functions routinely
take BigInt arguments in addition to "this". Those each need
reachabilityFences, even for those that currently have one-liner
implementations. Often temporary BigInts are allocated, filled in, and
possibly returned as results. Those need reachabilityFences, just in case
the result is not used. The rules for constructors are not 100% clear to
me. But I think those, counterintuitively, also generally need
reachabilityFences to deal with the case in which the protected field is
not subsequently used. I have little confidence I could get this right
without a number of iterations.

I agree that it would be good to make the JDK reachabilityFence-correct.
It's the only available tool, so it's necessary for correctness. But I
would also view it as a feasibility experiment, keeping in mind that the
hardest code to deal with is probably in third-party libraries. My guess is
that we haven't gotten to the final solution here.

On Sat, Apr 15, 2017 at 8:51 AM, Gil Tene <gil at azul.com> wrote:

> IMO, so long as the JDK's own performance-sensitive methods do not follow
> this advice, it will be hard to "educate" people to use reachabilityFence
> responsibly.
>
> To be specific:
>
> As long as java.nioDirectByteBuffer.get() is effectively coded like this
> (after the templates are applied):
>
>     public byte get() {
>         return (unsafe.getByte(ix(nextGetIndex())));
>     }
>
> Instead of like this:
>
>     public byte get() {
>         try {
>                 return (unsafe.getByte(ix(nextGetIndex())));
>         } finally {
>                 Reference.reachabilityFence(this);
>         }
>     }
>
> People will copy it and extend on it, replicating the reachability bug and
> the use-after-free problem in the current code.
>
> It is hard to ask people to code "responsibly" when the JDK itself
> doesn't. This is especially true when the bug in the above code is not
> obvious, and (as Hans points out earlier) it takes a long time to convince
> people it actually exists. This is especially true since the "well, when
> was the last time someone complained about DirectByteBuffer.get() doing
> getting a SEGV on access to a freed buffer?" argument holds some water. And
> that's where you actually end up usually when discussing the above missing
> reachabilityFence…
>
> A few things are keeping the JDK code from evolving to properly use
> Reference.reachabilityFence(this):
>
> a. It didn't exist until recently. [That's a very good excuse]
>
> b. While Reference.reachabilityFence exists now, it's implementation is
> sub-optimal. IUUC it is modeled [currently] as an opaque method call, which
> prohibits enough useful optimizations to significantly hurt
> DirectByteBuffer.get() if it were actually included in the code right now.
>
> c. As Hans notes below, the compilers may temporarily dodge this issue by
> inhibiting dead variable elimination for references. They do so to allow
> the actual (semantically buggy) current JDK code to execute safely and
> performantly.
>
> As long as the compilers paper over this by temporarily dodging the issue
> and inhibiting dead variable elimination for references, we are giving
> people a continued excuse to ignore the need for adding reachabilityFences
> to their code. And this situation will probably continue (for practical
> purposes) at least as long as using reachabilityFence in the above common
> form is not optimized in practice to be equivalent to just inhibiting dead
> variable elimination of (this) across the same scope [and not just
> implemented as a much more expensive opaque method effect].
>
> This makes the subject somewhat premature for wide public education IMO. A
> practical set of steps might look like this:
>
> 0. Add Reference.reachabilityFence() to the platform [this is basically
> the existing "collect underpants" step]
>
> 1. make try { } finally { Reference.reachabilityFence(this); } optimize
> well (to be no more expensive than inhibiting dead variable elimination for
> 'this' across the try block).
>
> 2. Change JDK code to actually make use of this code idiom in the various
> places where it is actually needed for correctness.
>
> 3. Relax compiler limitation crutches that currently help make the
> correctness bugs not trigger often. But do so first with a non-default -XX
> flag, and later transition to default with a backout flag (since a lot of
> user code will start breaking when the crutch is taken away).
>
> 4. Educate and evangelize for proper use of Reference.reachabilityFence(this);
> (maybe even add a method annotation for people who like that sort of thing?
> E.g. @AlwaysReachableThis)
>
> 5. Profit
>
>
> > On Apr 1, 2017, at 10:19 AM, Hans Boehm <hboehm at google.com> wrote:
> >
> > That also sounds fine to me.
> >
> > The difficulty with applying this, especially in the JNI case, is that
> you
> > may need several reachability fences at the end of each method, guarding
> > parameters and temporaries, not all of which may be naturally in scope at
> > the end. But we're not going to be able to include a full discussion
> here.
> >
> > In the interest of full disclosure, we currently, at least temporarily,
> > dodge this issue by inhibiting dead variable elimination for references.
> >
> >
> > On Apr 1, 2017 01:55, "Andrew Haley" <aph at redhat.com> wrote:
> >
> > On 31/03/17 19:56, Hans Boehm wrote:
> >> This method should be used when cleanup actions (finalize() calls, or
> >> Reference enqueuing, Cleaner invocation) could otherwise be triggered
> > while
> >> a resource under control of the object is still in use. This method
> should
> >> normally be called when these cleanup facilities are used to perform
> >> actions other than simply issuing a warning.
> >
> > It's still pretty confusing.  It would be simpler to say that
> > reachabilityFence should be used at the end of every method of a class
> > which has cleanup actions if you don't want those cleanup actions to
> > be run before the end of that method.  In that case, reachabilityFence
> > only makes any difference in cases where otherwise you'd have a bug.
> >
> > Andrew.
>
>