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.redes...@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-99206970363698485...@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 <hbo...@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

Reply via email to