During initialization of a shared object, the object must be accessible only to the thread constructing it. However, the object can be published safely (that is, made visible to other threads) once its initialization is complete. The Java memory model (JMM) allows multiple threads to observe the object after its initialization has begun but before it has concluded. Consequently, programs must prevent publication of partially initialized objects.

This rule prohibits publishing a reference to a partially initialized member object instance before initialization has concluded. It specifically applies to safety in multithreaded code. TSM01-J. Do not let the this reference escape during object construction prohibits the this reference of the current object from escaping its constructor. OBJ11-J. Be wary of letting constructors throw exceptions describes the consequences of publishing partially initialized objects even in single-threaded programs.

Noncompliant Code Example

This noncompliant code example constructs a Helper object in the initialize() method of the Foo class. The Helper object's fields are initialized by its constructor.

class Foo {
  private Helper helper;

  public Helper getHelper() {
    return helper;
  }

  public void initialize() {
    helper = new Helper(42);
  }
}

public class Helper {
  private int n;

  public Helper(int n) {
    this.n = n;
  }
  // ...
}

If a thread were to access helper using the getHelper() method before the initialize() method executed, the thread would observe an uninitialized helper field. Later, if one thread calls initialize() and another calls getHelper(), the second thread could observe one of the following:

  • The helper reference as null
  • A fully initialized Helper object with the n field set to 42
  • A partially initialized Helper object with an uninitialized n, which contains the default value 0

In particular, the JMM permits compilers to allocate memory for the new Helper object and to assign a reference to that memory to the helper field before initializing the new Helper object. In other words, the compiler can reorder the write to the helper instance field and the write that initializes the Helper object (that is, this.n = n) so that the former occurs first. This can expose a race window during which other threads can observe a partially initialized Helper object instance.

There is a separate issue: if more than one thread were to call initialize(), multiple Helper objects would be created. This is merely a performance issue—correctness would be preserved. The n field of each object would be properly initialized and the unused Helper object (or objects) would eventually be garbage-collected.

Compliant Solution (Synchronization)

Appropriate use of method synchronization can prevent publication of references to partially initialized objects, as shown in this compliant solution:

class Foo {
  private Helper helper;

  public synchronized Helper getHelper() {
    return helper;
  }

  public synchronized void initialize() {
    helper = new Helper(42);
  }
}

Synchronizing both methods guarantees that they cannot execute concurrently. If one thread were to call initialize() just before another thread called getHelper(), the synchronized initialize() method would always finish first. The synchronized keywords establish a happens-before relationship between the two threads. Consequently, the thread calling getHelper() would see either the fully initialized Helper object or an absent Helper object (that is, helper would contain a null reference). This approach guarantees proper publication both for immutable and mutable members.

Compliant Solution (Final Field)

The JMM guarantees that the fully initialized values of fields that are declared final are safely published to every thread that reads those values at some point no earlier than the end of the object's constructor.

class Foo {
  private final Helper helper;

  public Helper getHelper() {
    return helper;
  }

  public Foo() {
    // Point 1
    helper = new Helper(42);
    // Point 2
  }
}

However, this solution requires the assignment of a new Helper instance to helper from Foo's constructor. According to The Java Language Specification, §17.5.2, "Reading Final Fields During Construction" [JLS 2015]:

A read of a final field of an object within the thread that constructs that object is ordered with respect to the initialization of that field within the constructor by the usual happens-before rules. If the read occurs after the field is set in the constructor, it sees the value the final field is assigned; otherwise, it sees the default value.

Consequently, the reference to the helper instance should remain unpublished until the Foo class's constructor has completed (see TSM01-J. Do not let the this reference escape during object construction for additional information).

Compliant Solution (Final Field and Thread-Safe Composition)

Some collection classes provide thread-safe access to contained elements. When a Helper object is inserted into such a collection, it is guaranteed to be fully initialized before its reference is made visible. This compliant solution encapsulates the helper field in a Vector<Helper>.

class Foo {
  private final Vector<Helper> helper;

  public Foo() {
    helper = new Vector<Helper>();
  }

  public Helper getHelper() {
    if (helper.isEmpty()) {
      initialize();
    }
    return helper.elementAt(0);
  }

  public synchronized void initialize() {
    if (helper.isEmpty()) {
      helper.add(new Helper(42));
    }
  }
}

The helper field is declared final to guarantee that the vector is always created before any accesses take place. It can be initialized safely by invoking the synchronized initialize() method, which ensures that only one Helper object is ever added to the vector. If invoked before initialize(), the getHelper() avoids the possibility of a null-pointer dereference by conditionally invoking initialize(). Although the isEmpty() call in getHelper() is made from an unsynchronized context (which permits multiple threads to decide that they must invoke initialize) race conditions that could result in addition of a second object to the vector are nevertheless impossible. The synchronized initialize() method also checks whether helper is empty before adding a new Helper object, and at most one thread can execute initialize() at any time. Consequently, only the first thread to execute initialize() can ever see an empty vector and the getHelper() method can safely omit any synchronization of its own.

Compliant Solution (Static Initialization)

In this compliant solution, the helper field is initialized statically, ensuring that the object referenced by the field is fully initialized before its reference becomes visible:

// Immutable Foo
final class Foo {
  private static final Helper helper = new Helper(42);

  public static Helper getHelper() {
    return helper;
  }
}

The helper field should be declared final to document the class's immutability.

According to JSR-133, Section 9.2.3, "Static Final Fields" [JSR-133 2004]:

The rules for class initialization ensure that any thread that reads a static field will be synchronized with the static initialization of that class, which is the only place where static final fields can be set. Thus, no special rules in the JMM are needed for static final fields.

Compliant Solution (Immutable Object - Final Fields, Volatile Reference)

The JMM guarantees that any final fields of an object are fully initialized before a published object becomes visible [Goetz 2006a]. By declaring n final, the Helper class is made immutable. Furthermore, if the helper field is declared volatile in compliance with VNA01-J. Ensure visibility of shared references to immutable objects, Helper's reference is guaranteed to be made visible to any thread that calls getHelper() only after Helper has been fully initialized.

class Foo {
  private volatile Helper helper;

  public Helper getHelper() {
    return helper;
  }

  public void initialize() {
    helper = new Helper(42);
  }
}

// Immutable Helper
public final class Helper {
  private final int n;

  public Helper(int n) {
    this.n = n;
  }
  // ...
}

This compliant solution requires that helper be declared volatile and that class Helper is immutable. If the helper field were not volatile, it would violate VNA01-J. Ensure visibility of shared references to immutable objects.

Providing a public static factory method that returns a new instance of Helper is both permitted and encouraged. This approach allows the Helper instance to be created in a private constructor.

Compliant Solution (Mutable Thread-Safe Object, Volatile Reference)

When Helper is mutable but thread-safe, it can be published safely by declaring the helper field in the Foo class volatile:

class Foo {
  private volatile Helper helper;

  public Helper getHelper() {
    return helper;
  }

  public void initialize() {
    helper = new Helper(42);
  }
}

// Mutable but thread-safe Helper
public class Helper {
  private volatile int n;
  private final Object lock = new Object();

  public Helper(int n) {
    this.n = n;

  }

  public void setN(int value) {
    synchronized (lock) {
      n = value;
    }
  }
}

Synchronization is required to ensure the visibility of mutable members after initial publication because the Helper object can change state after its construction. This compliant solution synchronizes the setN() method to guarantee the visibility of the n field.

If the Helper class were synchronized incorrectly, declaring helper volatile in the Foo class would guarantee only the visibility of the initial publication of Helper; the visibility guarantee would exclude visibility of subsequent state changes. Consequently, volatile references alone are inadequate for publishing objects that are not thread-safe.

If the helper field in the Foo class is not declared volatile, the n field must be declared volatile to establish a happens-before relationship between the initialization of n and the write of Helper to the helper field. This is required only when the caller (class Foo) cannot be trusted to declare helper volatile.

Because the Helper class is declared public, it uses a private lock to handle synchronization in conformance with LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

Exceptions

TSM03-J-EX0: Classes that prevent partially initialized objects from being used may publish partially initialized objects. This could be implemented, for example, by setting a volatile Boolean flag in the last statement of the initializing code and checking whether the flag is set before allowing class methods to execute.

The following compliant solution shows this technique:

public class Helper {
  private int n;
  private volatile boolean initialized; // Defaults to false

  public Helper(int n) {
    this.n = n;
    this.initialized = true;
  }

  public void doSomething() {
    if (!initialized) {
      throw new SecurityException(
          "Cannot use partially initialized instance");
    }
    // ...
  }
  // ...
}

This technique ensures that if a reference to the Helper object instance were published before its initialization was complete, the instance would be unusable because each method within Helper checks the flag to determine whether the initialization has finished.

Risk Assessment

Failure to synchronize access to shared mutable data can cause different threads to observe different states of the object or to observe a partially initialized object.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

TSM03-J

Medium

Probable

Medium

P8

L2

Automated Detection

ToolVersionCheckerDescription

Bibliography

[API 2006]


[Bloch 2001]

Item 48, "Synchronize Access to Shared Mutable Data"

[Goetz 2006a]

Section 3.5.3, "Safe Publication Idioms"

[Goetz 2007]

Pattern #2, "One-Time Safe Publication"

[JPL 2006]

Section 14.10.2, "Final Fields and Security"

[Pugh 2004]




45 Comments

    • Is the Date class the same as java.util.Date? If it is not meant to be a preexisting Date class I would use a different name for your example class.
    • The 1st NCCE is another one that is too general...the code shows no race condition between setDate() and getDate(). Prob an easy fix would be:
        if (f.getDate() != null) {
          f.setDate( Date.getCurrentDate());
        }
      
    • The first CS does not make the Foo class immutable; it just puts stricter controls on the mutability.
    • Wouldn't code that uses a non-volatile d object, but also has the setDate() and getDate() methods synchronized also be a valid CS?
    • In the 2nd CS, why make the d volatile if it is final?
    1. As if I haven't criticized this rule enough, one more problem: Declaring a mutable thread-unsafe object volatile is not a security flaw per se, it just doesn't make the same guarantees that a volatile primitive type gets. By that argument, you could also insist that arrays never be volatile (CON11-J).

      I would guess there might still be reasons to have a mutable or thread-unsafe volatile object, so I'm not (yet) convince that we should restrict volatile to primitive types. What do you think?

        • Not necessarily. Depends on your use case. The objective here is safe publication.
        • Yes same as java.util.Date.
        • The race condition is not between those two methods. The point is that the client may observe f.d == null even though f.d has already been set by another thread.
        • "Effectively immutable" as opposed to "immutable" means just that. Good observation.
        • Yes, sure. The technique I used for the CS is faster. See the reference for more information.
        • Yes, that seems like a copy-paste issue. I've corrected it.

        Wrt the other point, I am narrowing the title. Last but not the least, this guideline is only a couple of hours old so I'll let it evolve. Meanwhile, comments are welcome.

          • Not necessarily. Depends on your use case. The objective here is safe publication.

          Dhruv, allowing your NCCE to implicitly violate OBJ00-J indicates a lack of faith in OBJ00-J...it's a good idea, but only if you feel like it. We want these rules to be more rigorous and precise and we have to convince people that following these rules is critical for their security, even if they don't feel like it. That means the rules need to be tolerable to coders...and that includes ourselves. The phrase "eat our own dog food" is appropriate here. Your choices are:

          • Declare explicitly that your NCCE violates OBJ00-J, and list the reasons why. Ideally OBJ00-J should also have those reasons listed as exceptions.
          • Modify your NCCE, or OBJ00-J, in such a way that your NCCE no longer violates OBJ00-J.

          I'd prefer the latter, but I'll leave the choice up to you. Note that this applies to all fields in all codes in all the rules, not just this one. (but we can fix those later.)

          • Yes same as java.util.Date.

          That's a problem. The java.util.Date.setDate() method is deprecated since JDK 1.1. (and I know we have a rule against using deprecated methods.) Given the code samples you've got, I suggest you use another class.

          • The race condition is not between those two methods. The point is that the client may observe f.d == null even though f.d has already been set by another thread.

          OK, but that is not particularly bad as far as the code is concerned. The code needs to at least show both the f.d == null and the setting of f.d by that other thread. Right now there is no condition possible b/c no other thread has access to the f object.

          I reviewed the conversation we had about concrete vs. general examples, and the conclusion was "there is no black and white answer". So I'm saying you need a more concrete example for this rule. (without saying you need more concrete examples in general).

          • "Effectively immutable" as opposed to "immutable" means just that. Good observation.

          Huh? This is squishy thinking. In this case s/effectively immutable/mutable, but under strictly controlled circumstances/.

          • Yes, sure. The technique I used for the CS is faster. See the reference for more information.

          OK. I think it's worthwhile to have my CS listed before yours (that is, a CS with non-volatile d and synchronized set/get methods). Then your CS that uses volatile d and sync set method appears next and is noted to be faster than mine.

          Wrt the other point, I am narrowing the title. Last but not the least, this guideline is only a couple of hours old so I'll let it evolve. Meanwhile, comments are welcome.

          That's a start, but I'm not sure what "facilitating safe publication" means. I just know that declaring an object volatile doesn't provide the same guarantees as a volatile primitive type. Let me rephrase my earlier assertion as a question: Is there any good reason to declare an object volatile, provide it is mutable or thread-safe? It might be best to forbid any such declaration (which was your rule's original intent)...I just don't know.

          Finally, whatever we decide for volatile objects would also apply to volatile arrays, hence CON11-J. I still think CON11-J and this rule should be merged together.

          1. I have eliminated the idea of "safe publication" and generalized this guideline and explained some terms such as "effectively immutable". I hope these changes address your concerns.

            Is there any good reason to declare an object volatile, provide it is mutable or thread-safe?

            There may be. But this guideline has been renamed to not disallow declaring mutable variables as volatile, but to synchronize the setters. Bottom line: this guideline shows how synchronization is preferable over the use of volatile alone, to ensure visibility of objects, and only visibility. This is slightly different from CON01 whose main purpose is atomicity and a secondary purpose - visibility. It is different from CON00 because this one shows the pitfalls of volatile w/o synchronization when used with mutable objects. The advice of this guideline is not even an exception to CON00 because synchronization can be used in addition to volatile. These guidelines are complementary. I plan to name this one CON01 and the current CON01 as CON02. Comments?

            1. Personally, I'd keep all the IDs the same until we do one great renumbering, prob after we are done making our other changes to the Concurrency section.

              "effectively immutable" Goetz 07, that is, its state cannot be directly or indirectly changed after the object is initialized or published.

              • That sounds like a definition for the glossary. Is that an official term that Goetz uses?
              • The 'final' CS needs to initialize map to some useful value. The code currently leave it empty and final...not very useful. Perhaps the ctor should take a list of keys and values (or another map), and fill the map. At the very least the ctor needs some "// fill map with useful data" comment.
              • In the volatile CS, the first sentence should read:

              It follows from the previous compliant solution that it is safe to declare the map volatile if it is effectively immutable.

              • This is the 2nd rule that makes an object volatile, with a non-synchronized getter + a synchronized setter. That sounds like a pattern that deserves its own rule (or should be a CS in some current basic rule, CON02-J perhaps?
              • Finally, I still think this rule should be merged with CON11-J, but we can discuss this tomorrow.
                • Unofficial. I think it is useful.
                • Done
                • Done
                • That seems to be a performance thing. It may not require a separate guideline by itself.
                • ...
  1. From [JPL 06], 14.10.2. Final Fields and Security:

    ... indeed you can use final fields to define immutable objects. There is a common misconception that shared access to immutable objects does not require any synchronization because the state of the object never changes. This is a misconception in general because it relies on the assumption that a thread will be guaranteed to see the initialized state of the immutable object, and that need not be the case. The problem is that, while the shared object is immutable, the reference used to access the shared object is itself shared and often mutable. Consequently, a correctly synchronized program must synchronize access to that shared reference, but often programs do not do this, because programmers do not recognize the need to do it. For example, suppose one thread creates a String object and stores a reference to it in a static field. A second thread then uses that reference to access the string. There is no guarantee, based on what we've discussed so far, that the values written by the first thread when constructing the string will be seen by the second thread when it accesses the string.

    ... if a reference to an object is stored after the object has been constructed, then any thread that reads that reference is guaranteed to see initialized values for the object's final fields.

    What the second rule states, in general terms, is that if you read a reference from a final field, then any non-final fields in the referenced object will have a value at least as recent as the value they had when the reference was written. For example, this means that if one thread constructs an object and uses methods on that object to set its state, then a second thread will see the state set by the first thread if the reference used to access the object was written to a final field after the state changes were made.

    Points to note:

    • Objects (of Foo) that have final fields (such as Helper) that are published to other threads before their construction is over are not immutable.
    • Likewise, objects with static fields (Helper) may be vulnerable if they are published before object construction is over.

    I think these are similar to TSM01-J. Do not let the (this) reference escape during object construction. Just that you need an example with class Foo which has a constructor that initializes the helper field. Foo can be accessed by multiple threads. It is prematurely published.

    I also think the problem is with the words "fully constructed". The "full construction" should also mean that a reference to the object (Helper) was not published before Helper's own construction concluded.

    Problems with current guideline:

    • Compliant Solution (immutable) violates the quote from [JPL 06]. If one thread is in the process of initializing helper while another thread attempts to retrieve its instance, it may see a default value. Additional synchronization may be required or declaring the variable volatile may suffice.
    • Compliant Solution (static initialization) also seems to violate this quote. (see the String example). I think the field should be final and/or additional synchronization may be required.
    • Notably, JSR-133 FAQ [Manson 04] describes an example and says it is thread-safe (w/o the modifications), equivalently:
    public class Foo {
      private static class LazyHelperHolder {
        public static Helper helper = new Helper();
      }
    
      public static Helper getInstance() {
        return LazyHelperHolder.helper;
      }
    }
    
  2. From, [JSR-133 04]:

    Do not write a reference to the object being constructed in a place where another
    thread can see it before the object’s constructor is finished. If this is followed, then when the object is seen by another thread, that thread will always see the correctly constructed version of that object’s final fields.

    One way to have an NCE could be the following but it cannot be fixed. Publishing "this" before or after construction is immaterial.

    class Foo {
      private final Helper helper;
      public static Foo f;   
      public Helper getHelper() {
        return helper;
      }
     
      public Foo() {	  
        f = this;  
        helper = new Helper(42);    
      }
    }
    
    public class Tester {
      public static void main(String[] args) {
        System.out.println(Foo.f.getHelper()); // Foo is null and so is helper
      }
    }
    

    Can you think of something less contrived in which Foo is non-null but helper is null? I think the quote refers to:

    // helper is being constructed and has final fields (immutable)
    class Foo {
      private static Helper helper; // nonfinal
      public static Helper helper2; // violation, bad publish
       
      public Helper getHelper() { // instance method, not a bad publication
        return helper;
      }
     
      public Foo() {
        helper2 = helper;
        helper = new Helper(42);
      }
    }
    

    Yet, another example:

    // helper is being constructed and has final fields
    class Foo {
      private static Helper helper;
         
      public Helper getHelper() {
        return helper;
      }
     
      public Foo() {
        Bar.doSomething(helper);
        helper = new Helper(42);
      }
    }
    
    public class Bar {
      public static void doSomething(Helper help) {
        // ...	
      }
    }
    

    Also, static initializers are an exception:

    // Even if Foo is published, no thread can observe the default value of the final field
    // Run the same Tester class
    class Foo {
      private static final Helper helper;
      public static Foo f;   
    
      public Helper getHelper() {
        return helper;
      }
     
     static {
       f = new Foo();
       try {
         Thread.sleep(1000); // Always executes
       } catch (InterruptedException e) {
         // ...
       }
       helper = new Helper(42);
     }
    }
    
  3. In Compliant Solution (volatile): volatile can be used to safely publish only when the Helper object is fully constructed or when it is immutable. It may not be a CS by itself for this situation and the text does not indicate this currently.

    1. Problems with current guideline:

      • Compliant Solution (immutable) violates the quote from JPL 06. If one thread is in the process of initializing helper while another thread attempts to retrieve its instance, it may see a default value. Additional synchronization may be required or declaring the variable volatile may suffice.

      Agreed. We need the quote in order to 'invalidate' the code. But it should probably become a NCCE.

      • Compliant Solution (static initialization) also seems to violate this quote. (see the String example). I think the field should be final and/or additional synchronization may be required.

      No, the JPL quote is not relevant. The CS is good not because Helper is static, but rather because it is statically initialized. The JMM guarantees that static initialization ensures safe publication fully constructed objects.

      One way to have an NCE could be the following but it cannot be fixed. Publishing "this" before or after construction is immaterial.
      ...

      Not sure how this code is relevant. Foo.f is never initialized, and that trumps any other problems with this code.

      In Compliant Solution (volatile): volatile can be used to safely publish only when the Helper object is fully constructed or when it is immutable. It may not be a CS by itself for this situation and the text does not indicate this currently.

      I've been led to believe that volatile can be used to publish the object, without worrying about a partially-initialized object. Unfortunately, my copy of Goetz is not available right now...if I'm right, the CS clearly needs a citation that it is good.

      I still believe the CS would be good if we used an AtomicReference instead.

      1. No, the JPL quote is not relevant. The CS is good not because Helper is static, but rather because it is statically initialized. The JMM guarantees that static initialization ensures safe publication fully constructed objects.

        A thread can call getHelper() and see the field with default values. The CS cannot be insecure.

        Not sure how this code is relevant. Foo.f is never initialized, and that trumps any other problems with this code.

        This comment says that you must not publish the object prematurely. This ruins the guarantees provided by even final. This is similar to yet different from TSM01-J. Do not let the (this) reference escape during object construction because we are not letting the this reference of the current object escape, but the reference of the sub-object under construction. Perhaps this discussion belongs to CON14. This guideline should refer to COn14 in that case (esp. the CS final because its guarantees depend on whether the sub-object was prematurely published before initialization or not).

        I've been led to believe that volatile can be used to publish the object, without worrying about a partially-initialized object.

        Offhand, you can look at [Goetz 07] in the references, pattern #2. Also, see the last NCE of VNA06-J. Do not assume that declaring an object reference volatile guarantees visibility of its members. You'll spot the missing text that should go into CS volatile.

  4. Regarding my first point, is there a reason why the field is not declared final? It would be fully compliant in that case.

    According to JSR-133, 9.2.3 Static Final Fields -

    The rules for class initialization ensure that any thread that reads a static field will be synchronized with the static initialization of that class, which is the only place where static final fields can be set. Thus, no special rules in the JMM are needed for static final fields.

    This is safer than using just static fields. For example, if the CS had a setter then it would clearly be not thread-safe. The use of final tells us exactly that we cannot have a setter.

    1. I believe I have addressed these comments.

      1. It appears that Compliant Solution (thread-safe composition) violates VNA03-J. Do not assume that a group of calls to independently atomic methods is atomic. Between creating the vector and adding an element to it, a thread could call getHelper() and see the default value of the vector instead of the one after adding Helper(42). It should be possible to declare the vector final, initialize in the constructor and let the current initialize method just add the helper to the vector.

        class Foo {
          private final Vector<Helper> helper;
        
          public Helper getHelper() {
            return helper.elementAt(0);
          }
        
          Foo() {
            helper = new Vector<Helper>();  
          }
          public void initialize() {
            helper.add(new Helper(42));
          }
        }
        
  5. I think you can just remove the hard-coded number 42 and let the constructor accept the num. No need to check if helper is null, anywhere.

    1. OK, I think the code is ready for review.

      WRT your comment, the code would be best with either the null checks & non-hardcoded number, or w/o the null chekcs & with 42. The issue with the null checks is to prevent double initialization, which is a problem distinct from what this rule is talking about. So I opted for simplicity, and removed the null checks. While the rule mentions the double init'n, it should focus on the partial-construction problem.

  6. Dhruv has created a new CS, nicknamed 'volatile flag' that raises two problems:

    • I think the code is safe, but not sure. It depends on the fact that initialized defaults to false (according to the JLS) and is set to true when the constructor completes. Being volatile prevents it from being reordered by the compiler. My question is, if a second thread can see a partially-constructed Helper object, could it see initialized before it is initialized to false? I know a JVM would have to (1) allocate memory for the object, (2) initialize the fields (3) set the type of the object (to Helper) and (4) call the constructor. At what point could a partially-initialized object be seen? What guarantees do the JLS give us here?

    For instance, this would be a problem in C/C++ with pthreads...it is quite possible for a spying thread to see allocated-but-not-initialized memory. But this is mainly b/c C & C++ don't require initialization like Java does.

    • Assuming my above problem is unfounded and the code is indeed safe, it raises another problem. It abides by the spirit of this rule, but violates the letter. It does this by allowing a partially-constructed object to be published, but the object is in a known 'uninitialized' state, and is therefore safe. Our options are:
    • leave it as a CS
    • indicate that this is a valid exception to this rule
    • Modify the rule to permit this CS
    • Forbid the code, even though it is technically safe.

    IME Java concurrency rules always seem to have safe code that violates the rule, and trying to catch all corner cases tends to make for extremely complex rules. So I'm not in favor of modifying this rule. Offhand, I'd personally prefer that this be an exception to the rule, but what do others think?

    1. For the record, even though I added it as a CS I have a slight preference to list it as an exception (this code may be hard to maintain and not suitable for a CS in general cases). But what do others think?

      if a second thread can see a partially-constructed Helper object, could it see initialized before it is initialized to false?

      According to the description of the issue. a partially constructed object is when a thread observes n to be 0. This is the default value. Similarly, a thread can only see the default value of the boolean flag which will be false. The security of the solution lies in the fact that even if you obtain an unintialized Helper instance, you cannot invoke any methods on it because of the check.

      From JLS, 17.4.5 Happens-before Order:

      The default initialization of any object happens-before any other actions (other than default-writes) of a program.

      I think this is a fairly strong statement.

  7. We might want to list the public static factory method approach of CON14's Compliant Solution (public static factory method), as a solution/exception in this guideline.

    1. You're welcome to try adding the example. I don't think it will help, though.
      The problem is the initialize() method, whereas the factory method is suppsoed to wrap around a constructor.

      1. Foo will be unable to construct the object using its constructor because the constructor is private. So, it has to use the newInstance() method to get an instance of Helper. And this method guarantees that an object will be fully initialized before it is returned.

        1. That's what the initialize() method is for...to create the Helper object. In the NCCE it doesn't properly guarantee the object is fully initialized, and in the CS's it does. The most we could do is mention that this is similar to a factory method. Again, if you want to take a crack at it, go ahead.

          1. Done. I found one source: [Goetz 06]. See Listing 3.8.

  8. Jaroslav Sevcik says:

    I should also note that your "Compliant Solution (thread-safe
    composition)" example on the https://www.securecoding.cert.org/confluence/x/7ABQAg page looks very
    suspicious because there is a data race on the helper field between
    the getHelper and initialize methods.

    • We've established that the Factory Method CS in fact does allow a partially initialized object to be published. Please fix this (perhaps make it a NCCE.)
    • Looking at them now, I'm not sure why we need both the 'thread-safe composition' CS and the 'final + thread-safe composition' CS. They seem redundant...they should be merged or else the 2nd should justify its difference from the 1st.
      • The Factory method CS is safe because n is final. This was duly fixed before the discussion. However, helper can be declared volatile as suggested by everyone. This also means that it fits into Compliant Solution (immutable object, volatile reference). I think we should add to the latter CS that a factory method can be used as well instead of a constructor and completely get rid of the factory method CS.
      • The thread-safe composition CS without final is broken anyway. I think we should get rid of it.
      1. I got rid of it & merged its text into the following CS. Also beefed up that CS to handle a call to getHelper() preceding initialize().

        1. Suggest you change:

            public Helper getHelper() throws SecurityException {
              try {
                return helper.elementAt(0);
              } catch (ArrayIndexOutOfBoundsException e) {
                throw new SecurityException("Get called before initialize");
              }
            }
          
          

          to: check if the vector is empty. If not, return element(0). Catching runtime exceptions = questionable practice.

          1. Done. now getHelper() returns null if called b/f initialize().

            1. I think now you violate MET10-J. For methods that return an array or collection prefer returning an empty array or collection over a null value. How about -

              public Helper getHelper() {
                if(helper.isEmpty()) {
                  initialize();
                } 
                return helper.elementAt(0);	 
              }
              

              Two threads may still see helper as empty but only one will win while trying to add the vector element because initialize() is synchronized.

  9. David,

    The title Compliant Solution (immutable object, factory method)

    is not a very good fit. I had combined it with CS (immutable object, volatile reference) because using a public static factory method was used to guarantee immutability of the object. You still need the reference to be declared volatile. This CS is not distinct from Compliant Solution (immutable object, volatile reference).

    Also, the quote:

    A read of a final field of an object within the thread that constructs that object is ordered with respect to the initialization of that field within the constructor by the usual happens-before rules. If the read occurs after the field is set in the constructor, it sees the value the final field is assigned, otherwise it sees the default value.

    should occur where it was because this was there to support the sentence "The reference to the helper field should not be published before class Foo's constructor has finished its initialization (see CON14-J. Do not let the "this" reference escape during object construction).".

    Also, some of your edits in EX1 do not mean what I want them to mean. We can discuss next time if you like.

    1. I struck a compromise. Where I couldn't, I left hidden comments. Feel free to go over them and leave me a comment or edit accordingly. Thanks!

  10. A condition can be added to some of the CS classes' getHelper() method such that initialize() is called when helper is null. Subsequently, a non-null Helper can be returned.

  11. Another possibility is to add an example where the object is published before deserialization has fully constructed the object. For example, in a custom deserialized form using writeObject(), the writeObject() method should be synchronized.

  12. This rule cannot be violated if Helper is immutable; perhaps we can use this fact to replace some of the CS's here with just one saying "make Helper immutable". (This comment arose from a discussion on VNA01-J. Ensure visibility of shared references to immutable objects)

    1. I replied in CON09.

  13. It seems to me that the Compliant Solution for final field and thread-safe composition is a solution to a different problem than the one represented by the NCE.

  14. To be consistent with the title, I prefer the term "partially-constructed" be changed to "partially initialized".

  15. Thomas Hawtin sez:

    final fields are safe to publish at the end of the constructor of their class. The object may be a subclass of the class containing the field.

  16. In the "CS (Mutable Thread Safe Object, Volatile Reference)", the synchronized block in setN() is completely unnecessary since the Helper "n" field is volatile.

  17. In "Compliant Solution (Final Field)" it is said: "the JMM guarantees that the fully initialized values of fields that are declared final are safely published to every thread that reads those values at some point no later than the end of the object's constructor."

    From what I understand, I would have written "... at some point no EARLIER than the end of the object's constructor".

    I cannot understand what it would otherwise mean.

    1. Oops, you're right. I adopted your suggestion.