<div dir="ltr"><div>> To me it seems like an escaping this-reference is safe if the following condition is fulfilled: One can make sure that the this-reference is only stored by the method to which it is passed as argument, it is not used in any other way before the constructor has completed.</div><div><br></div><div>The method or class has to be final. Otherwise, a deriving class can override and now "this" escapes.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 17, 2023 at 5:55 PM Jens Lideström <<a href="mailto:jens.lidestrom@fripost.org">jens.lidestrom@fripost.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 2022-12-30 17:48, Brian Goetz wrote:<br>
> One thing that makes me a little uncomfortable is that @SW usually is used to say “the compiler can’t prove this is safe, but I am asserting it is.” But this is different; the compiler has discovered the class is unsafe,<br>
<br>
Is an escaping this-reference really always an unacceptable problem?<br>
<br>
To me it seems like an escaping this-reference is safe if the following condition is fulfilled: One can make sure that the this-reference is only stored by the method to which it is passed as argument, it is not used in any other way before the constructor has completed.<br>
<br>
Looking at the diff in Archie's branch [1], it seems like most cases are not problematic.<br>
<br>
[1]: <a href="https://github.com/openjdk/jdk/compare/master...archiecobbs:jdk:ThisEscape" rel="noreferrer" target="_blank">https://github.com/openjdk/jdk/compare/master...archiecobbs:jdk:ThisEscape</a><br>
<br>
In most cases in the diff the this-reference escapes to be able to do the following:<br>
<br>
* Set up a circular relationship with another object.<br>
* Register a Cleaner.<br>
* Register a listener.<br>
<br>
Sometimes an object might not be considered to be properly constructed without doing such things. It seems useful to be able to rely on the constructor to complete them.<br>
<br>
Regards,<br>
Jens Lideström<br>
<br>
On 2022-12-30 17:48, Brian Goetz wrote:<br>
> Nice.<br>
> <br>
> One thing that makes me a little uncomfortable is that @SW usually is used to say “the compiler can’t prove this is safe, but I am asserting it is.” But this is different; the compiler has discovered the class is unsafe, and @SW is saying “I know, I know, stop telling me.” So I think there’s an additional documentation opportunity here where, for the classes we had to annotate with @SW, we include an additional @apiNote of the form “This class behaves badly, oops”, with a link to a common doc page explaining the problem. (While we don’t have to do this necessarily immediately, the chance of it falling on the floor if we don’t are higher.). Basically, what you’ve identified is that there are a number of classes in the JDK which fail to follow best practices with regard to self-use from constructors. We can’t turn back the clock, but we can mark these as “don’t code like my brother”, so people see these patterns of coding and realize they are not to be emulated.<br>
> <br>
> I also see in the code that there are two separate categories, LeakingThisInConstructor and this-escape; what’s the difference?<br>
> <br>
> <br>
> <br>
>> On Dec 29, 2022, at 11:10 PM, Archie Cobbs <<a href="mailto:archie.cobbs@gmail.com" target="_blank">archie.cobbs@gmail.com</a> <mailto:<a href="mailto:archie.cobbs@gmail.com" target="_blank">archie.cobbs@gmail.com</a>>> wrote:<br>
>><br>
>> On Wed, Dec 28, 2022 at 4:31 PM Archie Cobbs <<a href="mailto:archie.cobbs@gmail.com" target="_blank">archie.cobbs@gmail.com</a> <mailto:<a href="mailto:archie.cobbs@gmail.com" target="_blank">archie.cobbs@gmail.com</a>>> wrote:<br>
>><br>
>> So fortunately it looks feasible to address these with @SuppressWarnings("this-escape") annotations.<br>
>><br>
>> Adding them is probably a good thing in its own right, because they would serve as new "Heads up for possible 'this' escape" markers in the code where there were none before, especially because this gotcha can be pretty hard to spot.<br>
>><br>
>> I'll work on refining the 'this' escape patch to use @SuppressWarnings instead of build flags, and then update this thread when that's done.<br>
>><br>
>><br>
>> OK I have a draft of this. It's still a "draft" because I don't have the ability to build locally for every platform, so I'm having trouble provoking all the warnings from Java code that is platform-specific. And the automated github builds take a long time and then only report one more new warning, making for an impossibly slow feedback loop (if anyone is interested in helping out, that would be awesome and it's very easy).<br>
>><br>
>> In any case, the good news is that fewer @SupppressWarnings annotations than I thought are required. In part this is because there were some redundant warnings being generated, which is now fixed.<br>
>><br>
>> Here are the updated stats. These numbers are promisingly low.<br>
>><br>
>> The first column is the number of @SupppressWarnings annotations added to eliminate all of the new 'this' escape warnings I'm able to provoke.<br>
>><br>
>> #SUPPRESS #FILES MODULE<br>
>> 47 3077 java.base<br>
>> 0 128 java.compiler<br>
>> 0 18 java.datatransfer<br>
>> 86 2899 java.desktop<br>
>> 0 10 java.instrument<br>
>> 0 23 java.logging<br>
>> 4 330 java.management<br>
>> 0 16 java.management.rmi<br>
>> 5 199 java.naming<br>
>> 15 142 java.net.http<br>
>> 0 20 java.prefs<br>
>> 4 106 java.rmi<br>
>> 0 15 java.scripting<br>
>> 0 1 <a href="http://java.se" rel="noreferrer" target="_blank">java.se</a> <<a href="https://urldefense.com/v3/__http://java.se__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLj5kvW3Ws$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__http://java.se__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLj5kvW3Ws$</a>><br>
>> 7 216 java.security.jgss<br>
>> 0 30 java.security.sasl<br>
>> 0 23 java.smartcardio<br>
>> 1 77 java.sql<br>
>> 0 56 java.sql.rowset<br>
>> 0 5 java.transaction.xa<br>
>> 39 1848 java.xml<br>
>> 32 271 java.xml.crypto<br>
>> 3 20 jdk.accessibility<br>
>> 0 21 jdk.attach<br>
>> 0 18 jdk.charsets<br>
>> 53 333 jdk.compiler<br>
>> 1 76 jdk.crypto.cryptoki<br>
>> 0 35 <a href="http://jdk.crypto.ec" rel="noreferrer" target="_blank">jdk.crypto.ec</a> <<a href="https://urldefense.com/v3/__http://jdk.crypto.ec__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjMLnBEws$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__http://jdk.crypto.ec__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjMLnBEws$</a>><br>
>> 0 11 jdk.crypto.mscapi<br>
>> 4 68 jdk.dynalink<br>
>> 0 3 jdk.editpad<br>
>> 10 942 jdk.hotspot.agent<br>
>> 3 56 jdk.httpserver<br>
>> 0 5 jdk.incubator.concurrent<br>
>> 0 50 jdk.incubator.vector<br>
>> 0 3 jdk.internal.ed<br>
>> 1 61 jdk.internal.jvmstat<br>
>> 1 113 jdk.internal.le<br>
>> 1 52 jdk.internal.opt<br>
>> 5 209 <a href="http://jdk.internal.vm.ci" rel="noreferrer" target="_blank">jdk.internal.vm.ci</a> <<a href="https://urldefense.com/v3/__http://jdk.internal.vm.ci__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjlAiOBII$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__http://jdk.internal.vm.ci__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjlAiOBII$</a>><br>
>> 0 1 jdk.internal.vm.compiler<br>
>> 0 1 jdk.internal.vm.compiler.management<br>
>> 0 18 jdk.jartool<br>
>> 5 228 jdk.javadoc<br>
>> 0 41 jdk.jcmd<br>
>> 18 64 jdk.jconsole<br>
>> 10 124 jdk.jdeps<br>
>> 25 254 jdk.jdi<br>
>> 0 1 jdk.jdwp.agent<br>
>> 0 263 jdk.jfr<br>
>> 0 77 jdk.jlink<br>
>> 1 80 jdk.jpackage<br>
>> 6 88 jdk.jshell<br>
>> 0 4 jdk.jsobject<br>
>> 1 11 jdk.jstatd<br>
>> 0 281 jdk.localedata<br>
>> 1 25 jdk.management<br>
>> 0 19 jdk.management.agent<br>
>> 0 15 jdk.management.jfr<br>
>> 0 16 jdk.naming.dns<br>
>> 0 8 jdk.naming.rmi<br>
>> 0 11 <a href="http://jdk.net" rel="noreferrer" target="_blank">jdk.net</a> <<a href="https://urldefense.com/v3/__http://jdk.net__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjKWHeJYA$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__http://jdk.net__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjKWHeJYA$</a>><br>
>> 0 2 jdk.nio.mapmode<br>
>> 0 11 jdk.random<br>
>> 0 37 jdk.sctp<br>
>> 0 30 jdk.security.auth<br>
>> 0 16 jdk.security.jgss<br>
>> 0 9 jdk.unsupported<br>
>> 0 8 jdk.unsupported.desktop<br>
>> 0 94 jdk.xml.dom<br>
>> 1 14 jdk.zipfs<br>
>><br>
>> Code available here <<a href="https://urldefense.com/v3/__https://github.com/archiecobbs/jdk/tree/ThisEscape__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjPRWM0ZM$" rel="noreferrer" target="_blank">https://urldefense.com/v3/__https://github.com/archiecobbs/jdk/tree/ThisEscape__;!!ACWV5N9M2RV99hQ!L6fbZinPZagPgDKlgLerhi4QrI3gEtnjcVISBKwwhyNWx6vQLuwL2nKnjRMGg4yMeBEbWn6k5OGYUlLjPRWM0ZM$</a>>.<br>
>><br>
>> -Archie<br>
>><br>
>> -- <br>
>> Archie L. Cobbs<br>
> <br>
</blockquote></div>