Is SharedSecrets thread-safe?

[Peter Levart]

On 12/31/20 2:30 AM, Hans Boehm wrote:
> It sounds as though this would be correct if
>
> if (static_field == null) {
>    initialize();
> }
> return static_field;
> ```
>
> were rewritten as
>
> Foo my_local_copy = static_field;
> if (my_copy == null) {
>     initialize();
>     my_local_copy = static_field;
> }
> return my_local_copy;
>
> That would prevent the redundant read of static_field unless this thread
> did the initialization, in which case the original null would no longer be
> visible to the second read.
>
> Hans


I agree. The initialize() part is triggering some class initialization 
where concurrent attempts do establish a HB edge so the 2nd read of 
static_field after initialize() is ordered properly and can't read null.

I created a JIRA ticket here: 
https://bugs.openjdk.java.net/browse/JDK-8259021

And prepared a PR here: https://github.com/openjdk/jdk/pull/1914


Happy new year,

Peter


>
> On Tue, Dec 29, 2020 at 4:55 PM Claes Redestad <claes.redestad at oracle.com>
> wrote:
>
>> Hans' assessment seems about right in the generic case he's describing.
>>
>> But consider:
>>
>> 1. There is no concurrent setting of anything here - it's done in a
>> static initializer which will happen exactly once by the thread
>> initializing the class ($ 12.2.4 item 9).
>>
>> 2. While there is a data race on the access of the fields in
>> SharedSecrets, all of the Access instances are stateless. This means the
>> race is benign in the sense that when reading the field only a null or
>> a safely published instance can be observed.
>>
>> I wouldn't swear there's a strict guarantee a null can never be returned
>> from a SharedSecrets accessor though. Perhaps that's something that
>> could be hardened.
>>
>> /Claes
>>
>> On 2020-12-30 00:32, some-java-user-99206970363698485155 at vodafonemail.de
>> wrote:
>>> That would also be my understanding of the current situation, though
>> this contradicts what
>>> Claes wrote.
>>> Maybe the JVM behaves in a way which does not allow reordering, but the
>> JLS definitely seems
>>> to allow it. Section § 12.2.4 [0] only mentions that for the class to be
>> initialized there
>>> has to exist a lock LC (or at least the happens-before relationship),
>> but there is no
>>> "freeze the world" or anything similar which would force a
>> happens-before relationship
>>> for the code in `SharedSecrets`.
>>>
>>> Maybe most of the `SharedSecrets` methods are thread-safe (albeit
>> extremely brittle) because
>>> the classes to which the accessor objects belong to have previously
>> already been loaded
>>> before `SharedSecrets` is used, therefore having already established a
>> happens-before
>>> relationship.
>>> However, this is definitely not the case for all of the methods as shown
>> by the following
>>> example:
>>> ```
>>> CookieHandler.setDefault(new CookieHandler() {
>>>       @Override
>>>       public void put(URI uri, Map<String, List<String>> responseHeaders)
>> throws IOException { }
>>>       @Override
>>>       public Map<String, List<String>> get(URI uri, Map<String,
>> List<String>> requestHeaders) throws IOException {
>>>           return Collections.emptyMap();
>>>       }
>>> });
>>>
>>> // Any site which uses cookies (i.e. Set-Cookie or Set-Cookie2 header)
>>> URL url = new URL("https://oracle.com");
>>> url.openConnection().getHeaderFields();
>>> ```
>>>
>>> Running this code with `openjdk 15 2020-09-15` shows that the call to
>>> `SharedSecrets.getJavaNetHttpCookieAccess()` is made before the class
>> `HttpCookie` has
>>> been accessed and initialized. Therefore merely running this code in two
>> separate threads
>>> (both having been started before the code is executed, since
>> `Thread.start()` establishes
>>> a happens-before relationship) should be enough to render that
>> `SharedSecrets` method
>>> non-thread-safe.
>>>
>>> Kind regards
>>>
>>>
>>> [0]
>> https://docs.oracle.com/javase/specs/jls/se15/html/jls-12.html#jls-12.4.2
>>>> Hans Boehm <hboehm at google.com> hat am 29. Dezember 2020 um 18:53
>> geschrieben:
>>>> If static_field is not volatile, and set concurrently, then the first
>> read of static_field may return non-null and the second null, without
>> initialize() even being executed. The Java memory model does not prevent
>> reordering of non-volatile reads from the same field (for good reason).
>>>> Even if initialize() is executed and performs a volatile read, this
>> reasoning doesn't hold. The initial static_field read may be delayed past
>> the volatile read inside the conditional and hence, at least theoretically,
>> past the second read. Control dependencies don't order reads, either in
>> Java, or in modern weakly-ordered architectures with branch prediction.
>> This doesn't matter if initialize() sets static_field.
>>>> This all assumes that having two threads call initialize() is OK.
>>>>
>>>> Java code with data races is extremely tricky and rarely correct.
>>>>
>>>> Hans