Is it worth keeping the discussion starting with "It is sometimes possible to better encapsulate ..." and the associated example code? I find this example extremely unconvincing. It's very hard to construct a case in which you can safely use the result of getExternalResource(). And I don't want to encourage writing getters for things like this; if you use them from a static method, or somebody decides to make it public, things get really messy really quickly.
This example kind of gives the impression that we have a solution to reachabilityFence() proliferation. I don't think we do. There may be a difference of opinion about whether it's worth fixing, but I don't think we should deny the existence of the problem. On Thu, Mar 21, 2024 at 7:11 PM David Holmes <dhol...@openjdk.org> wrote: > On Thu, 21 Mar 2024 23:38:30 GMT, Brent Christian <bchri...@openjdk.org> > wrote: > > >> Classes in the `java.lang.ref` package would benefit from an update to > bring the spec in line with how the VM already behaves. The changes would > focus on _happens-before_ edges at some key points during reference > processing. > >> > >> A couple key things we want to be able to say are: > >> - `Reference.reachabilityFence(x)` _happens-before_ reference > processing occurs for 'x'. > >> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the > registered cleaning action. > >> > >> This will bring Cleaner in line (or close) with the memory visibility > guarantees made for finalizers in [JLS 17.4.5]( > https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5 > ): > >> _"There is a happens-before edge from the end of a constructor of an > object to the start of a finalizer (§12.6) for that object."_ > > > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > > > Elaborate on 'surprising and undesirable effects' in > reachabilityFence() > > src/java.base/share/classes/java/lang/ref/Reference.java line 576: > > > 574: * detects that there is no further need for an object. The > garbage collector > > 575: * may reclaim an object even if values from that object's > fields are still > > 576: * in use, or while a method on the object is still running, so > long as the > > Suggestion: method _of_ the object > > src/java.base/share/classes/java/lang/ref/Reference.java line 579: > > > 577: * object has otherwise become unreachable. > > 578: * <p> > > 579: * This may have surprising and undesirable effects, in > particular when using > > Nit: I don't think a new paragraph is appropriate here as "This may ..." > refers back to the subject of the preceding lines. > > src/java.base/share/classes/java/lang/ref/Reference.java line 581: > > > 579: * This may have surprising and undesirable effects, in > particular when using > > 580: * a Cleaner or finalizer for cleanup. If an object becomes > unreachable while > > 581: * a method on the object is running, it can lead to a race > between the > > Again suggest: method _of_ the object > > src/java.base/share/classes/java/lang/ref/Reference.java line 585: > > > 583: * Cleaner or finalizer. For instance, the cleanup thread could > cleanup the > > 584: * resource, followed by the program thread (still running the > method) > > 585: * attempting to access the now-already-freed resource. > > suggestion: s/cleanup/free/ to match the `now-already-freed` part. > > src/java.base/share/classes/java/lang/ref/Reference.java line 586: > > > 584: * resource, followed by the program thread (still running the > method) > > 585: * attempting to access the now-already-freed resource. > > 586: * {@code reachabilityFence} can prevent this race by ensuring > that the > > Suggestion: `Use of {@code reachabilityFence ...` > > This avoids starting a sentence with a code font word that starts with a > lower-case letter. > > ------------- > > PR Review Comment: > https://git.openjdk.org/jdk/pull/16644#discussion_r1534946886 > PR Review Comment: > https://git.openjdk.org/jdk/pull/16644#discussion_r1534947996 > PR Review Comment: > https://git.openjdk.org/jdk/pull/16644#discussion_r1534948233 > PR Review Comment: > https://git.openjdk.org/jdk/pull/16644#discussion_r1534948894 > PR Review Comment: > https://git.openjdk.org/jdk/pull/16644#discussion_r1534949965 >
