RFR 9: 8165641 : Deprecate Object.finalize

[Gil Tene]

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.