У меня проблема с переупорядочением, и это из-за обращения к ссылке?

У меня есть этот класс, где я кеширую экземпляры и клонирую их (данные изменяемы), когда я их использую.

Интересно, могу ли я столкнуться с проблемой переупорядочения с этим.

Я посмотрел этот ответ и JLS, но я все еще не уверен.

public class DataWrapper {
    private static final ConcurrentMap<String, DataWrapper> map = new ConcurrentHashMap<>();
    private Data data;
    private String name;

    public static DataWrapper getInstance(String name) {
        DataWrapper instance = map.get(name);
        if (instance == null) {
            instance = new DataWrapper(name);
        }
        return instance.cloneInstance();
    }

    private DataWrapper(String name) {
        this.name = name;
        this.data = loadData(name);  // A heavy method
        map.put(name, this);  // I know
    }

    private DataWrapper cloneInstance() {
        return new DataWrapper(this);
    }

    private DataWrapper(DataWrapper that) {
        this.name = that.name;
        this.data = that.data.cloneInstance();
    }
}

Мое мышление: Среда выполнения может изменить порядок оператора в конструкторе и опубликовать текущий экземпляр DataWrapper (помещенный на карту) перед инициализацией объекта data. Второй поток читает экземпляр DataWrapper из карты и видит поле null data (или частично построен).

Возможно ли это? Если да, то только из-за обращения ссылки?

Если нет, не могли бы вы объяснить мне, как рассуждать о происшествии - до согласованности в более простых терминах?

Что делать, если я сделал это:

public class DataWrapper {
    ...
    public static DataWrapper getInstance(String name) {
        DataWrapper instance = map.get(name);
        if (instance == null) {
            instance = new DataWrapper(name);
            map.put(name, instance);
        }
        return instance.cloneInstance();
    }

    private DataWrapper(String name) {
        this.name = name;
        this.data = loadData(name);     // A heavy method
    }
    ...
}

Разве это все еще подвержено той же проблеме?

Обратите внимание, что я не против, если есть один или два дополнительных экземпляра, созданных, если несколько потоков пытаются создать и поместить экземпляр для одного и того же значения одновременно.

EDIT:

Что делать, если поля имени и данных были окончательными или изменчивыми?

public class DataWrapper {
    private static final ConcurrentMap<String, DataWrapper> map = new ConcurrentHashMap<>();
    private final Data data;
    private final String name;
    ... 
    private DataWrapper(String name) {
        this.name = name;
        this.data = loadData(name);  // A heavy method
        map.put(name, this);  // I know
    }
    ...
}

Это все еще небезопасно? Насколько я понимаю, гарантии безопасности инициализации конструктора применимы только в том случае, если ссылка не сбежала во время инициализации. Я ищу официальные источники, подтверждающие это.

Ответ 1

Если вы хотите быть spec-compliant, вы не можете применить этот конструктор:

private DataWrapper(String name) {
  this.name = name;
  this.data = loadData(name);
  map.put(name, this);
}

Как вы заметили, JVM разрешено изменить порядок на следующее:

private DataWrapper(String name) {
  map.put(name, this);
  this.name = name;
  this.data = loadData(name);
}

При назначении значения в поле final это подразумевает так называемое действие замораживания в конце конструктора. Модель памяти гарантирует, что произойдет до отношения между этим действием замораживания и любой разменкой экземпляра, на который было применено это действие замораживания. Однако это соотношение существует только в конце конструктора, поэтому вы нарушаете это соотношение. Перетаскивая публикацию из своего конструктора, вы можете исправить эту реальность.

Если вы хотите более формальный обзор этих отношений, я рекомендую просматривать этот набор слайдов. Я также объяснил отношение в этой презентации, начиная примерно с минуты 34.

Ответ 2

Реализация имеет некоторые очень тонкие оговорки.

Кажется, вы знаете, но для того, чтобы быть ясным, в этом фрагменте кода несколько потоков могут получить экземпляр null и ввести блок if, без необходимости создавать новые экземпляры DataWrapper:

public static DataWrapper getInstance(String name) {
    DataWrapper instance = map.get(name);
    if (instance == null) {
        instance = new DataWrapper(name);
    }
    return instance.cloneInstance();
}

Кажется, вы в порядке с этим, но это требует предположения, что loadData(name) (используется DataWrapper(String)) всегда будет возвращать одно и то же значение. Если он может возвращать разные значения в зависимости от времени, нет гарантии, что последний поток для загрузки данных сохранит его в map, поэтому значение может быть устаревшим. Если вы скажете, что этого не произойдет, или это не важно, это прекрасно, но это предположение должно быть как минимум документировано.

Чтобы продемонстрировать еще одну тонкую проблему, позвольте мне встроить метод instance.cloneInstance():

public static DataWrapper getInstance(String name) {
    DataWrapper instance = map.get(name);
    if (instance == null) {
        instance = new DataWrapper(name);
    }
    return new DataWrapper(instance);
}

Тонкая проблема здесь заключается в том, что этот оператор возврата не является безопасной публикацией. Новый экземпляр DataWrapper может быть частично сконструирован, и нить может наблюдать его в несогласованном состоянии, например, поля объекта еще не установлены.

Для этого есть простое исправление: если вы сделаете поля name и data final, класс становится неизменным. Неизменяемые классы пользуются специальными гарантиями инициализации, и return new DataWrapper(this); становится безопасной публикацией.

С этим простым изменением и при условии, что вы в порядке с первой точкой (loadData не зависит от времени), я думаю, что реализация должна работать правильно.

Я бы рекомендовал дополнительное усовершенствование, не связанное с правильностью, но другие хорошие практики. В нынешней реализации слишком много обязанностей: это обертка вокруг data и в то же время кэш. Добавленная ответственность заставляет его немного запутывать чтение. И в стороне, параллельная хэш-карта на самом деле не используется для ее потенциала.

Если вы разделяете обязанности, результат может быть проще, лучше и проще для чтения:

class DataWrapperCache {

  private static final ConcurrentMap<String, DataWrapper> map = new ConcurrentHashMap<>();

  public static DataWrapper get(String name) {
    return map.computeIfAbsent(name, DataWrapper::new).defensiveCopy();
  }
}

class DataWrapper {

  private final String name;
  private final Data data;

  DataWrapper(String name) {
    this.name = name;
    this.data = loadData(name);  // A heavy method
  }

  private DataWrapper(DataWrapper that) {
    this.name = that.name;
    this.data = that.data.cloneInstance();
  }

  public DataWrapper defensiveCopy() {
    return new DataWrapper(this);
  }
}