Does the program remain correct when I drop one of two volatiles?

Question

I've got this simplified code snippet with two volatiles (assume we must keep both fields):

volatile boolean hasParam;
volatile String param;

boolean hasParam() {
  if (param == null) {
    getParam();
  }
  return hasParam;
}

String getParam() {
  String tmp = param;
  if (tmp == null) {
    tmp = "String";
    hasParam = true;
    param = tmp;
  }
  return tmp;
}

I'm speculating whether I can drop volatile from hasParam field declaration in this way: if there are two threads calling hasParam() under race and the first one writes into volatile param field (doing releasing store) and the second thread reads non-null value from the same field (doing acquiring read) then it must read true and only true from hasParam. In the opposite case (acquiring read reads null) the second thread writes true into param and returns it from hasParam().

Basing on this speculation I assume I can safely erase volatile keyword from hasParam field declaration.

In order to prove it I've designed this JCStress-based test:

@State
@JCStressTest
@Outcome(id = "true, String", expect = ACCEPTABLE, desc = "Boolean value was flushed")
@Outcome(id = "false, String", expect = FORBIDDEN, desc = "Boolean value was not flushed")
public class ConcurrencyTest {
  Value value = new Value();

  @Actor
  public void actor1(ZL_Result r) {
    r.r1 = value.hasParameter();
    r.r2 = value.param;
  }

  @Actor
  public void actor2(ZL_Result r) {
    r.r1 = value.hasParameter();
    r.r2 = value.param;
  }

  static class Value {

    volatile boolean hasParam;
    volatile String param;

    boolean hasParameter() {
      if (param == null) {
        getParam();
      }
      return hasParam;
    }

    String getParam() {
      String tmp = param;

      if (tmp == null) {
        tmp = "String";
        hasParam = true;
        param = tmp;
      }

      return tmp;
    }
  }
}

This test yields the results:

       RESULT      SAMPLES     FREQ      EXPECT  DESCRIPTION
false, String            0    0,00%   Forbidden  Boolean value was not flushed
 true, String  638 164 992  100,00%  Acceptable  Boolean value was flushed

If I erase volatile from hasParam field declaration then I get the same results (I assume it proves correctness of my speculation):

       RESULT        SAMPLES     FREQ      EXPECT  DESCRIPTION
false, String              0    0,00%   Forbidden  Boolean value was not flushed
 true, String  1 074 450 432  100,00%  Acceptable  Boolean value was flushed

And if I erase volatile from both field declaration the test fails with:

       RESULT        SAMPLES     FREQ      EXPECT  DESCRIPTION
false, String      1 420 423    0,12%   Forbidden  Boolean value was not flushed
 true, String  1 164 432 249   99,88%  Acceptable  Boolean value was flushed

So I've got two questions:

Is my speculation correct and the program remains correct?
Is my test correct?

This snipette does not look correct to me even with two `volatile`s. Right now it "works" because String is immutable and there's only one copy of the literal `"String"` in the system. But if you put anything else in its place, the system could easily execute `getParam()` twice in parallel and you'd get two copies of the resource made and usually that spells trouble. In short, don't to this, I think the only way to keep two variables and guarantee single resource creation is with mutual exclusion and locking the code in question. — markspace, Jun 10 '22 at 17:20
What you're doing is called "Double Checked Locking" and here's the right way to do it (also buy a copy of Brain Goetz's book *Java Concurrency in Practice,* it's explained in detail there): https://www.baeldung.com/java-singleton-double-checked-locking — markspace, Jun 10 '22 at 17:23
More help, "Double Checked Locking is Broken": https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html — markspace, Jun 10 '22 at 17:23
This SO link: https://stackoverflow.com/questions/18093735/double-checked-locking-in-singleton — markspace, Jun 10 '22 at 17:25
@markspace thanks for replying, my question is mostly whether I can keep existing behaviour with suggested change. — Sergey Tsypanov, Jun 10 '22 at 18:52
I would have to vote "no," you can't use this at all. This doesn't work for anything besides a string literal. Pleases check the links I gave you and use a proper pattern for lazy initialization. (And by "this" I mean both patterns you show are broken. Please never do either of these in production code.) — markspace, Jun 10 '22 at 20:57
Why do you have the `hasParam` field in the first place? It appears you could omit it and just use the `param == null` check. Also with both fields being `volatile` to me it looks like `hasParameter()` would always return true (unless there is an assignment to `Value.param` not shown in your code), either because the caller itself initializes `param`, or because another thread initializes it. Then the question is why `hasParameter()` is needed in the first place. Might be good if you provided code which is closer to your real code. — Marcono1234, Jun 11 '22 at 18:46
@markspace, regarding "Double Checked Locking is Broken"; the most important point is that it is _not broken anymore_ since Java 5, see the section "Under the new Java Memory Model" of that page. — Marcono1234, Jun 11 '22 at 18:48
The Java 5 version of that code that works uses a `synchronized` block to make it work. The OP doesn't have that, so the OPs code is broken. And "Double Checked Locking is Broken" refers to the version without the synchronized block. — markspace, Jun 11 '22 at 20:51
@Marcono1234 both fields are used in different methods, so we must keep both of them — Sergey Tsypanov, Jun 12 '22 at 07:22
“both fields are used in different methods”—what do you mean with “different”, other than the two posted here? The two methods shown don’t need two fields. And when there are other methods accessing these fields, there is no sense in asking us whether `volatile` can be removed and not showing *all* accesses to the fields. — Holger, Jun 14 '22 at 16:27
@Holger yes, this is a part of the code and we mustn't throw any of those fields. `hasParam` field is indeed accessed only from one method and I don't wont to drop it and rely on null check because it would imply volatile access. Instead I want to make sure that I can safely drop volatile from `hasParam` field declaration making sure the behavior remains the same. — Sergey Tsypanov, Jun 14 '22 at 16:56
What do you mean with “accessed only from one method”? Even in the code you’ve posted, `hasParam` is accessed by two methods. Do you mean by “one more” method or are you talking about the obsolete usage within `hasParam()` with has the `volatile` `null` check anyway? — Holger, Jun 15 '22 at 06:58
By “accessed only from one method” I mean accessed from outside of `Value` class. — Sergey Tsypanov, Jun 15 '22 at 08:40
And how are we supposed to judge whether the access from outside of the `Value` class will work when you remove the `volatile` modifier? A stress test that doesn’t include that access at all, can’t spot potential problems either, besides the fact that a result of a stress test can only hint but not prove whether code is correct. — Holger, Jun 16 '22 at 10:33
We have the test for it which is run three times: with 2 volatiles, without one and without both. The baseline is the case with 2 volatiles. If the test results remain the same having one volatile removed, then we can prove correctness of the transformation. And actually the test contains this line `r.r1 = value.hasParameter();` allowing us to make sure that even in case of racy access the correct value is returned. — Sergey Tsypanov, Jun 16 '22 at 10:55
In your example, the flag is always true when the array has been initialized to a non-`null` value, so `hasParam()` always returns `true`. That’s why these questions about the necessity of the field came up. In general, it’s a bad strategy to post such an oversimplified example and [asking three days later what the real flag actually means](https://stackoverflow.com/q/72606177/2711488). Posting the real case right from the beginning could have saved us a lot of time. — Holger, Jun 17 '22 at 10:04

Does the program remain correct when I drop one of two volatiles?

0 Answers0