#1283 DC_DOUBLECHECK is not a bug.

3.x
closed-rejected
William Pugh
5
2014-09-22
2014-06-14
Claude Martin
No

I really don't have much to say about this. It simply isn't a bug. It was in Java 1.4.
But this is 2014 and we use Java 8. I double checking all the time and there's no problem.
Sure, some may still use Java 1.4 but it's their problem. Unless the code compliance level is set to 1.4 it should just ignore it.

Related

Bugs: #1283

Discussion

    • Group: 3.0.0 --> 3.0.1
     
  • William Pugh
    William Pugh
    2014-09-19

    • status: open --> closed-rejected
    • assigned_to: William Pugh
     
  • William Pugh
    William Pugh
    2014-09-19

    Double checked locking is still a bug. In all versions of Java.

    You can declare the variable as volatile, and then use double checked locking. Or use any of a number of other strategies for initialize-once synchronization.

     
  • Claude Martin
    Claude Martin
    2014-09-20

    You can declare the variable as volatile,

    Sure you can. It's not always necessary. But I now understand why it would be rather hard for FB to check the code if it works without volatile. I understand it that it is only needed if you assign the return value of some method to the variable. So it doesn't matter of it as a primitive type. it's often a constructor that is called and the JIT would inline it. So the reference is assigned before the object is initialized. Volatile will prevent that.

    So if this really is a bug, will the explanation be updated? The text is now rather misleading. It should state that this works with Java 5+ (thanks to JMM) but should only be used on a variable that is primitive or volatile.
    The link to http://www.cs.umd.edu/... doesn't really help when someone wants to fix the bug.
    Maybe link to this text:
    http://www.javamex.com/tutorials/double_checked_locking_fixing.shtml
    Or this:
    https://blogs.oracle.com/cwebster/entry/double_check_locking

     
  • William Pugh
    William Pugh
    2014-09-21

    Give me an example of "double-checked locking" that you think is valid and that FindBugs complains about. I've gotten lots of such examples from many people, and in every single case the person was wrong and the code wasn't valid. You might be the first to actually come up with valid code where FindBugs complains about double checked locking.

     
  • Claude Martin
    Claude Martin
    2014-09-22

    I just think the text is misleading and out of date. I'd love to find an example, but I simply can't reproduce the bug even with example code that is known to be invalid. How could I find a valid piece of code if I can't even find invalid code? As I understand it this only happens with a combination of inlined code and values being partially committed to main memory.

    Now the bug description claims this:

    This method may contain an instance of double-checked locking. This idiom is not correct according to the semantics of the Java memory model.

    Double-checked locking is correct. But only on member variables that are primitive (except long and double) or volatile.

     
    • William Pugh
      William Pugh
      2014-09-23

      No, double checked locking isn’t correct. You are misinformed.

      It isn’t correct in Java 4, 5, 6, 7, 8 or 9.
      It isn’t correct in C or C++.

      Bill

      On Sep 22, 2014, at 7:20 AM, Claude Martin claude_m@users.sf.net wrote:

      I just think the text is misleading and out of date. I'd love to find an example, but I simply can't reproduce the bug even with example code that is known to be invalid. How could I find a valid piece of code if I can't even find invalid code? As I understand it this only happens with a combination of inlined code and values being partially committed to main memory.

      Now the bug description claims this:

      This method may contain an instance of double-checked locking. This idiom is not correct according to the semantics of the Java memory model.
      Double-checked locking is correct. But only on member variables that are primitive (except long and double) or volatile.

      [bugs:#1283] http://sourceforge.net/p/findbugs/bugs/1283 DC_DOUBLECHECK is not a bug.

      Status: closed-rejected
      Group: 3.x
      Labels: DC_DOUBLECHECK Java 1.4 JMM
      Created: Sat Jun 14, 2014 05:38 PM UTC by Claude Martin
      Last Updated: Sun Sep 21, 2014 01:22 PM UTC
      Owner: William Pugh

      I really don't have much to say about this. It simply isn't a bug. It was in Java 1.4.
      But this is 2014 and we use Java 8. I double checking all the time and there's no problem.
      Sure, some may still use Java 1.4 but it's their problem. Unless the code compliance level is set to 1.4 it should just ignore it.

      Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/findbugs/bugs/1283/ https://sourceforge.net/p/findbugs/bugs/1283
      To unsubscribe from further messages, please visit https://sourceforge.net/auth/subscriptions/ https://sourceforge.net/auth/subscriptions

       

      Related

      Bugs: #1283

      • Claude Martin
        Claude Martin
        2014-09-25

        Then why does FB accept DCL on primitive/volatile members?! DCL works in Java 5+. In all my tests it even works without volatile. But tests don't prove anything, and I don't claim it works without volatile.
        But the bug description is outdated and misleading. Why cant the description actually explain how to fix the bug? Just add volatile and it's fine.

         
        • Tagir Valeev
          Tagir Valeev
          2014-11-13

          DCL doesn't work without volatile even for primitives. You probably don't understand the Java Memory Model. If you don't specify the volatile keyword, either JIT compiler or CPU is allowed to reorder the instructions in any way which doesn't produce visible behavior changes within the current thread. This doesn't mean that your JVM and your CPU in your test will actually use this right, but your code may break on another JVM or another architecture or under another circumstances. Reordering is very useful for optimization: for example, CPU may wait dozens of cycles till cache line is filled from the main memory to execute the next instruction (main memory access is damn slow). So instead of idling CPU may analyze the following instruction and if they could be executed within currently cached data and don't break the current thread logic, it will execute them in advance. Consider for example the following code:

          String str;
          boolean init = false;
          
          void init() {
              if(!init) {
                  synchronized(this) {
                      if(!init) {
                          str = "initialized";
                          init = true;
                      }
                  }
              }
          }
          

          JIT and CPU are allowed to reorder string assignment and init assignment (despite init is primitive), because this doesn't break the thread logic. So you actually may encounter the case that you executed the init() method, but string is still not assigned. Though it would be really hard to write a test to catch this.

          If you specify volatile, then you bind volatile variable reads and writes in different threads with happens-before relation. You explicitly says that everything which occurs in threadA before writing the volatile variable must be visible in threadB after reading the same variable. So now both JIT compiler and CPU cannot reorder the instructions to violate this relation (for CPU it's achieved using memory barrier instruction).

          This bug pattern is somewhat similar to "RR: Method ignores results of InputStream.read() (RR_NOT_CHECKED)". You may write dozen of tests which will prove that is.read(bytes) always fills the whole array in your code. But by contract it's allowed not to do this, so your code may break under unusual circumstances (for example, if you provide a named pipe instead of regular file as the input).

           
          • Claude Martin
            Claude Martin
            2014-11-13

            ok, so we can agree that DCL is a bad idea. JSR133 states that it is a "dubious coding idiom". I posted this report because I was confused by the bug description and it is still confusing and outdated. Some might think (as I did) that this is just outdated and DCL works in Java 5+.