It looks like since jdk 1.5, making the array reference volatile will
prevent re-ordering array element writes (with respect to the volatile
array reference write):
https://web.archive.org/web/20200923063441/https://www.ibm.com/developerworks/library/j-jtp03304/index.html
So the code as initially written (sorry!) was not thread-safe, and the
solution initially proposed ("The fix would be declaring the array
volatile") would indeed fix the problem? i.e.:
private volatile int[] cachedOrdIdxMap;
private int[] getOrdIdxMap(...) {
final int[] local = cachedOrdIdxMap;
if (local != null) {
return local;
} else {
return cachedOrdIdxMap = compute(...); // idempotent (same result for
same input)
}
}
On Wed, Mar 17, 2021 at 8:11 AM Uwe Schindler <u...@thetaphi.de> wrote:
> Hi,
>
> I agree with Dawid to make it developer friendly. Most synchronized-blocks
> are rmeoved by Hotspot anyways, so better safe than sorry!
> David: I missed in your original mail that you were afraid of changes
> inside the array not being visible. You can prevent that with fences, but
> all of that is hard to understand.
>
> IMHO, I would use an AtomicReference:
>
> final AtomicReference<V> arr = new AtomicReference<>();
> public V getLazy() {
> V result = arr.get();
> if (result == null) {
> result = compute(...);
> if (!arr.compareAndSet(null, result)) {
> return arr.get();
> }
> }
> return result;
> }
>
> -----
> Uwe Schindler
> Achterdiek 19, D-28357 Bremen
> https://www.thetaphi.de
> eMail: u...@thetaphi.de
>
> > -----Original Message-----
> > From: Dawid Weiss <dawid.we...@gmail.com>
> > Sent: Wednesday, March 17, 2021 9:01 AM
> > To: dev@solr.apache.org
> > Subject: Re: Thread-safety without volatile for an immutable array
> >
> > Unless this code is used millions of millions of times I would just use a
> > code pattern that is understood by any developer - senior and junior... A
> > volatile or even initialization under a plain monitor. Those fancy memory
> > guards are great for low-level data structures but they are really hard
> to
> > understand (even for folks who have been using Java for long).
> >
> > D.
> >
> > On Wed, Mar 17, 2021 at 4:22 AM David Smiley <dsmi...@apache.org> wrote:
> >
> > > Moreover, AFAICT, in SharedSecrets, those fields were all immutable --
> no
> > > state. I spot checked a couple and saw no fields at all. Even final
> > > fields would be fine because of "effectively final" classes (e.g.
> String).
> > > But an int array is another matter; no?
> > >
> > > ~ David Smiley
> > > Apache Lucene/Solr Search Developer
> > > http://www.linkedin.com/in/davidwsmiley
> > >
> > >
> > > On Tue, Mar 16, 2021 at 6:30 PM Uwe Schindler <u...@thetaphi.de> wrote:
> > >
> > > > Hi,
> > > >
> > > > it is theoretically possible (but unlikely). There was a discussion
> on
> > > the
> > > > mailinglist of openjdk, because SharedSecrets had the same problem.
> > > >
> > > > There's a workaround for that: Use a local variable (copy value to a
> > > local
> > > > variable, then test for null and run the initialization)! See the
> fix for
> > > > SharedSecrets in the JDK:
> > > > https://github.com/openjdk/jdk/commit/85bac8c4
> > > > The trick is here: The order must be ensured in the actual thread,
> so the
> > > > local variable is exactly what you expect and cannot suddenly change
> to
> > > > null by reordering. So when you copy the global to a local and return
> > > that
> > > > one at end, you know that it's as you expect.
> > > >
> > > > You could also add a fence, so the order is preserved, I think for
> that
> > > > the following is fine (put it before the write inside the if clause):
> > > > <
> > > >
> > >
> >
> https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/VarHandle.html#a
> > cquireFence--
> > > > >
> > > >
> > > > I'd go with what the JDK developers did.
> > > >
> > > > Uwe
> > > >
> > > > -----
> > > > Uwe Schindler
> > > > Achterdiek 19, D-28357 Bremen
> > > > https://www.thetaphi.de
> > > > eMail: u...@thetaphi.de
> > > >
> > > > > -----Original Message-----
> > > > > From: David Smiley <dsmi...@apache.org>
> > > > > Sent: Tuesday, March 16, 2021 8:56 PM
> > > > > To: dev@solr.apache.org
> > > > > Cc: Michael Gibney <mich...@michaelgibney.net>
> > > > > Subject: Thread-safety without volatile for an immutable array
> > > > >
> > > > > I'm code reviewing something over here:
> > > > >
> > > > > https://github.com/apache/solr/pull/2/files#diff-
> > > > >
> > aa2f75bf39a49e1fcd52aacff89da3ea93f622803eb5996d5edb5e04070a50b1R6
> > > > > 58
> > > > >
> > > > > In a nutshell, it looks like this:
> > > > >
> > > > > private int[] cachedOrdIdxMap;
> > > > >
> > > > > private int[] getOrdIdxMap(...) {
> > > > > if (cachedOrdIdxMap == null) {
> > > > > cachedOrdIdxMap = compute(...); // idempotent (same result
> for
> > > same
> > > > > input)
> > > > > }
> > > > > return cachedOrdIdxMap;
> > > > > }
> > > > >
> > > > > I suspect this is not thread-safe because the value is not
> "published"
> > > > > safely, and thus it might be possible for a reader to see a
> > > > > non-null cachedOrdIdxMap yet it might not be populated yet because
> the
> > > > > machine is allowed to re-order reads and writes in the absence of
> any
> > > > > synchronization controls. The fix would be declaring the array
> > > > volatile, I
> > > > > think.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > ~ David Smiley
> > > > > Apache Lucene/Solr Search Developer
> > > > > http://www.linkedin.com/in/davidwsmiley
> > > >
> > > >
> > >
>
>
