Is SharedSecrets thread-safe?

[Hans Boehm]

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

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
>