Ensuring safe publication and thread safety in java by means of static factories

Question

The class below is meant to be immutable (but see edit):

public final class Position extends Data {

    double latitude;
    double longitude;
    String provider;

    private Position() {}

    private static enum LocationFields implements
            Fields<Location, Position, List<Byte>> {
        LAT {

            @Override
            public List<byte[]> getData(Location loc, final Position out) {
                final double lat = loc.getLatitude();
                out.latitude = lat;
                // return an arrayList
            }

            @Override
            public void parse(List<Byte> list, final Position pos)
                    throws ParserException {
                try {
                    pos.latitude = listToDouble(list);
                } catch (NumberFormatException e) {
                    throw new ParserException("Malformed file", e);
                }
            }
        }/* , LONG, PROVIDER, TIME (field from Data superclass)*/;

    }

    // ========================================================================
    // Static API (factories essentially)
    // ========================================================================
    public static  Position saveData(Context ctx, Location data) 
            throws IOException {
        final Position out = new Position();
        final List<byte[]> listByteArrays = new ArrayList<byte[]>();
        for (LocationFields bs : LocationFields.values()) {
            listByteArrays.add(bs.getData(data, out).get(0));
        }
        Persist.saveData(ctx, FILE_PREFIX, listByteArrays);
        return out;
    }

    public static List<Position> parse(File f) throws IOException,
            ParserException {
        List<EnumMap<LocationFields, List<Byte>>> entries;
        // populate entries from f
        final List<Position> data = new ArrayList<Position>();
        for (EnumMap<LocationFields, List<Byte>> enumMap : entries) {
            Position p = new Position();
            for (LocationFields field : enumMap.keySet()) {
                field.parse(enumMap.get(field), p);
            }
            data.add(p);
        }
        return data;
    }

    /**
     * Constructs a Position instance from the given string. Complete copy 
     * paste just to get the picture
     */
    public static Position fromString(String s) {
        if (s == null || s.trim().equals("")) return null;
        final Position p = new Position();
        String[] split = s.split(N);
        p.time = Long.valueOf(split[0]);
        int i = 0;
        p.longitude = Double.valueOf(split[++i].split(IS)[1].trim());
        p.latitude = Double.valueOf(split[++i].split(IS)[1].trim());
        p.provider = split[++i].split(IS)[1].trim();
        return p;
    }
}

Being immutable it is also thread safe and all that. As you see the only way to construct instances of this class - except reflection which is another question really - is by using the static factories provided.

Questions :

• Is there any case an object of this class might be unsafely published ?
• Is there a case the objects as returned are thread unsafe ?

EDIT : please do not comment on the fields not being private - I realize this is not an immutable class by the dictionary, but the package is under my control and I won't ever change the value of a field manually (after construction ofc). No mutators are provided.

The fields not being final on the other hand is the gist of the question. Of course I realize that if they were final the class would be truly immutable and thread safe (at least after Java5). I would appreciate providing an example of bad use in this case though.

Finally - I do not mean to say that the factories being static has anything to do with thread safety as some of the comments seem(ed) to imply. What is important is that the only way to create instances of this class is through those (static of course) factories.

Answer 1

Yes, instances of this class can be published unsafely. This class is not immutable, so if the instantiating thread makes an instance available to other threads without a memory barrier, those threads may see the instance in a partially constructed or otherwise inconsistent state.

The term you are looking for is effectively immutable: the instance fields could be modified after initialization, but in fact they are not.

Such objects can be used safely by multiple threads, but it all depends on how other threads get access to the instance (i.e., how they are published). If you put these objects on a concurrent queue to be consumed by another thread—no problem. If you assign them to a field visible to another thread in a synchronized block, and notify() a wait()-ing thread which reads them—no problem. If you create all the instances in one thread which then starts new threads that use them—no problem!

But if you just assign them to a non-volatile field and sometime "later" another thread happens to read that field, that's a problem! Both the writing thread and the reading thread need synchronization points so that the write truly can be said to have happened before the read.

Your code doesn't do any publication, so I can't say if you are doing it safely. You could ask the same question about this object:

class Option {

  private boolean value;

  Option(boolean value) { this.value = value; }

  boolean get() { return value; }

}

If you are doing something "extra" in your code that you think would make a difference to the safe publication of your objects, please point it out.

Answer 2

The fields of the Position class are not final, so I believe that their values are not safely published by the constructor. The constructor is therefore not thread-safe, so no code (such as your factory methods) that use them produce thread-safe objects.

Answer 3

Position is not immutable, the fields have package visibility and are not final, see definition of immutable classes here: http://www.javapractices.com/topic/TopicAction.do?Id=29.

Furthermore Position is not safely published because the fields are not final and there is no other mechanism in place to ensure safe publication. The concept of safe publication is explained in many places, but this one seems particularly relevant: http://www.ibm.com/developerworks/java/library/j-jtp0618/ There are also relevant sources on SO.

In a nutshell, safe publication is about what happens when you give the reference of your constructed instance to another thread, will that thread see the fields values as intended? the answer here is no, because the Java compiler and JIT compiler are free to re-order the field initialization with the reference publication, leading to half baked state becoming visible to other threads.

This last point is crucial, from the OP comment to one of the answers below he appears to believe static methods somehow work differently from other methods, that is not the case. A static method can get inlined much like any other method, and the same is true for constructors (the exception being final fields in constructors post Java 1.5). To be clear, while the JMM doesn't guarantee the construction is safe, it may well work fine on certain or even all JVMs. For ample discussion, examples and industry expert opinions see this discussion on the concurrency-interest mailing list: http://jsr166-concurrency.10961.n7.nabble.com/Volatile-stores-in-constructors-disallowed-to-see-the-default-value-td10275.html

The bottom line is, it may work, but it is not safe publishing according to JMM. If you can't prove it is safe, it isn't.