<div dir="ltr">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.<div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 21, 2024 at 7:11 PM David Holmes <<a href="mailto:dholmes@openjdk.org">dholmes@openjdk.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, 21 Mar 2024 23:38:30 GMT, Brent Christian <<a href="mailto:bchristi@openjdk.org" target="_blank">bchristi@openjdk.org</a>> wrote:<br>
<br>
>> 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.<br>
>> <br>
>> A couple key things we want to be able to say are:<br>
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing occurs for 'x'.<br>
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the registered cleaning action.<br>
>> <br>
>> This will bring Cleaner in line (or close) with the memory visibility guarantees made for finalizers in [JLS 17.4.5](<a href="https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5" rel="noreferrer" target="_blank">https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5</a>):<br>
>> _"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."_<br>
><br>
> Brent Christian has updated the pull request incrementally with one additional commit since the last revision:<br>
> <br>
> Elaborate on 'surprising and undesirable effects' in reachabilityFence()<br>
<br>
src/java.base/share/classes/java/lang/ref/Reference.java line 576:<br>
<br>
> 574: * detects that there is no further need for an object. The garbage collector<br>
> 575: * may reclaim an object even if values from that object's fields are still<br>
> 576: * in use, or while a method on the object is still running, so long as the<br>
<br>
Suggestion: method _of_ the object<br>
<br>
src/java.base/share/classes/java/lang/ref/Reference.java line 579:<br>
<br>
> 577: * object has otherwise become unreachable.<br>
> 578: * <p><br>
> 579: * This may have surprising and undesirable effects, in particular when using<br>
<br>
Nit: I don't think a new paragraph is appropriate here as "This may ..." refers back to the subject of the preceding lines.<br>
<br>
src/java.base/share/classes/java/lang/ref/Reference.java line 581:<br>
<br>
> 579: * This may have surprising and undesirable effects, in particular when using<br>
> 580: * a Cleaner or finalizer for cleanup. If an object becomes unreachable while<br>
> 581: * a method on the object is running, it can lead to a race between the<br>
<br>
Again suggest: method _of_ the object<br>
<br>
src/java.base/share/classes/java/lang/ref/Reference.java line 585:<br>
<br>
> 583: * Cleaner or finalizer. For instance, the cleanup thread could cleanup the<br>
> 584: * resource, followed by the program thread (still running the method)<br>
> 585: * attempting to access the now-already-freed resource.<br>
<br>
suggestion: s/cleanup/free/ to match the `now-already-freed` part.<br>
<br>
src/java.base/share/classes/java/lang/ref/Reference.java line 586:<br>
<br>
> 584: * resource, followed by the program thread (still running the method)<br>
> 585: * attempting to access the now-already-freed resource.<br>
> 586: * {@code reachabilityFence} can prevent this race by ensuring that the<br>
<br>
Suggestion: `Use of {@code reachabilityFence ...`<br>
<br>
This avoids starting a sentence with a code font word that starts with a lower-case letter.<br>
<br>
-------------<br>
<br>
PR Review Comment: <a href="https://git.openjdk.org/jdk/pull/16644#discussion_r1534946886" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/16644#discussion_r1534946886</a><br>
PR Review Comment: <a href="https://git.openjdk.org/jdk/pull/16644#discussion_r1534947996" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/16644#discussion_r1534947996</a><br>
PR Review Comment: <a href="https://git.openjdk.org/jdk/pull/16644#discussion_r1534948233" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/16644#discussion_r1534948233</a><br>
PR Review Comment: <a href="https://git.openjdk.org/jdk/pull/16644#discussion_r1534948894" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/16644#discussion_r1534948894</a><br>
PR Review Comment: <a href="https://git.openjdk.org/jdk/pull/16644#discussion_r1534949965" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/16644#discussion_r1534949965</a><br>
</blockquote></div>