You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@solr.apache.org by David Smiley <ds...@apache.org> on 2021/03/16 19:56:27 UTC
Thread-safety without volatile for an immutable array
I'm code reviewing something over here:
https://github.com/apache/solr/pull/2/files#diff-aa2f75bf39a49e1fcd52aacff89da3ea93f622803eb5996d5edb5e04070a50b1R658
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
Re: Thread-safety without volatile for an immutable array
Posted by David Smiley <ds...@apache.org>.
My preference is just to use volatile. AtomicReference is basically some
fancy wrapper on top of it that _in this case_ wouldn't add value. I
understand volatile.
~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley
On Wed, Mar 17, 2021 at 10:52 AM Michael Gibney <mi...@michaelgibney.net>
wrote:
> 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 <uw...@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: uwe@thetaphi.de
> >
> > > -----Original Message-----
> > > From: Dawid Weiss <da...@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 <ds...@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 <uw...@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: uwe@thetaphi.de
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: David Smiley <ds...@apache.org>
> > > > > > Sent: Tuesday, March 16, 2021 8:56 PM
> > > > > > To: dev@solr.apache.org
> > > > > > Cc: Michael Gibney <mi...@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
> > > > >
> > > > >
> > > >
> >
> >
>
Re: Thread-safety without volatile for an immutable array
Posted by Michael Gibney <mi...@michaelgibney.net>.
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 <uw...@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: uwe@thetaphi.de
>
> > -----Original Message-----
> > From: Dawid Weiss <da...@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 <ds...@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 <uw...@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: uwe@thetaphi.de
> > > >
> > > > > -----Original Message-----
> > > > > From: David Smiley <ds...@apache.org>
> > > > > Sent: Tuesday, March 16, 2021 8:56 PM
> > > > > To: dev@solr.apache.org
> > > > > Cc: Michael Gibney <mi...@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
> > > >
> > > >
> > >
>
>
RE: Thread-safety without volatile for an immutable array
Posted by Uwe Schindler <uw...@thetaphi.de>.
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: uwe@thetaphi.de
> -----Original Message-----
> From: Dawid Weiss <da...@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 <ds...@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 <uw...@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: uwe@thetaphi.de
> > >
> > > > -----Original Message-----
> > > > From: David Smiley <ds...@apache.org>
> > > > Sent: Tuesday, March 16, 2021 8:56 PM
> > > > To: dev@solr.apache.org
> > > > Cc: Michael Gibney <mi...@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
> > >
> > >
> >
Re: Thread-safety without volatile for an immutable array
Posted by Dawid Weiss <da...@gmail.com>.
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 <ds...@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 <uw...@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#acquireFence--
> > >
> >
> > I'd go with what the JDK developers did.
> >
> > Uwe
> >
> > -----
> > Uwe Schindler
> > Achterdiek 19, D-28357 Bremen
> > https://www.thetaphi.de
> > eMail: uwe@thetaphi.de
> >
> > > -----Original Message-----
> > > From: David Smiley <ds...@apache.org>
> > > Sent: Tuesday, March 16, 2021 8:56 PM
> > > To: dev@solr.apache.org
> > > Cc: Michael Gibney <mi...@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
> >
> >
>
Re: Thread-safety without volatile for an immutable array
Posted by David Smiley <ds...@apache.org>.
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 <uw...@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#acquireFence--
> >
>
> I'd go with what the JDK developers did.
>
> Uwe
>
> -----
> Uwe Schindler
> Achterdiek 19, D-28357 Bremen
> https://www.thetaphi.de
> eMail: uwe@thetaphi.de
>
> > -----Original Message-----
> > From: David Smiley <ds...@apache.org>
> > Sent: Tuesday, March 16, 2021 8:56 PM
> > To: dev@solr.apache.org
> > Cc: Michael Gibney <mi...@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
>
>
RE: Thread-safety without volatile for an immutable array
Posted by Uwe Schindler <uw...@thetaphi.de>.
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#acquireFence-->
I'd go with what the JDK developers did.
Uwe
-----
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail: uwe@thetaphi.de
> -----Original Message-----
> From: David Smiley <ds...@apache.org>
> Sent: Tuesday, March 16, 2021 8:56 PM
> To: dev@solr.apache.org
> Cc: Michael Gibney <mi...@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