Помогите просмотреть следующий код, является ли он потокобезопасным?

Question

private static Callback callback;

public Foo()
{
    super(getCallback());
}

private static Callback getCallback()
{
    callback = new Callback();
    return callback;
}

Конструктор Foo() потенциально может быть вызван из нескольких потоков.Меня беспокоит частное статическое поле 'обратный вызов' и статический метод 'getCallback()'.

Как можно видеть, каждый раз, когда вызывается 'getCallback()', он присваивает новое значение статическому полю 'обратный вызов'.

Мой догадываюсь заключается в том, что это не потокобезопасно, потому что ключевое слово статический всегда привязан к классу, а не к экземпляру, так что это означает, что статическое поле 'обратный вызов' Foo потенциально может быть перезаписано другим потоком, который создает другой Foo().Правильно ли это?

Пожалуйста, поправьте меня, если я ошибаюсь.Спасибо!

Редактировать:Мое намерение состоит в том, чтобы сохранить "обратный вызов" где-нибудь в классе, чтобы я мог повторно использовать его позже.Но это непросто, потому что Foo расширяется из класса, у которого есть конструктор, требующий передачи обратного вызова.

Answer 1

Да, вы правы.Это возможно для двух случаев Foo чтобы в итоге получить то же самое CallBack например, когда два потока входят в getCallback() метод одновременно и назначается новый CallBack к статическому полю, в то время как другой уже сделал это, но еще не вернулся.В этом случае лучшее решение - не иметь статического поля, поскольку оно не служит никакой цели.В качестве альтернативы, сделайте getCallback() синхронизировано.

Но обратите внимание, что это нет верно, что только static ключевое слово приводит к получению кода, который не является потокобезопасным.

Answer 2

Это не потокобезопасно.Попробуйте эти альтернативы:

Вариант 1:здесь все экземпляры используют один и тот же обратный вызов

private static final Callback callback = new Callback();

public Foo() {
    super(callback);
}

Вариант 2:здесь каждый экземпляр имеет свой собственный обратный вызов

public Foo() {
    super(new Callback());
}

Обратите внимание, что в обоих случаях, хотя конструктор потокобезопасен, потокобезопасность всего класса зависит от реализации обратного вызова.Если он имеет изменяемое состояние, то у вас возникнут потенциальные проблемы.Если обратный вызов неизменяем, то у вас есть потокобезопасность.

Answer 3

Обратный вызов будет получать новое значение каждый раз при вызове Foo() (даже из того же потока).Я не совсем уверен, что должен делать ваш код (если вы хотите инициализировать статическую переменную только один раз (singleton), вам следует проверить, по-прежнему ли она равна null в getCallback() - и что такое actionCallback?).Чтобы сделать его потокобезопасным, используйте synchronized .

Answer 4

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

Один очевидный вопрос заключается в том, делает ли callback должны быть статичными?Или вы могли бы безопасно сделать это полем экземпляра, не нарушая функциональность вашего класса?

Answer 5

Я знаю, что на этот вопрос был дан ответ, но "почему" на самом деле не было подробно объяснено.

Если два потока вызывают метод getCallback(), они могут выполнить строки следующим образом:

1. Поток 1 - обратный вызов = новый обратный вызов();
2. Поток 2 - обратный вызов = новый обратный вызов();
3. Поток 1 - возврат actionCallback;
4. Поток 2 - возврат actionCallback;

В этом случае обратный вызов, сгенерированный в (2.), будет возвращен как в (3.), так и в (4.)

Казалось бы, решение состоит в том, чтобы спросить, почему обратный вызов определяется статически, если он специфичен для экземпляра not class.

Я надеюсь, что это поможет.

Answer 6

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

private static final Callback CALLBACK= new Callback();

Или, если вам нужен ленивый синглтон, вы можете сделать

public class Foo {
   class CallbackHolder {
       static final Callback CALLBACK= new Callback();
   }

   public static Callback getCallback() {
      return CallbackHolder.CALLBACK;
   }

public Foo() {
    super(getCallback());
}

Обе реализации потокобезопасны.

Answer 7

Вам нужен один обратный вызов на поток, один на объект или настоящий синглтон?

Несколько набросков о том, как сделать различные варианты - просто с моей точки зрения, не воспринимайте это слишком буквально :)

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

По одному на поток:

  private static ThreadLocal<Callback> callback;

  public Foo()
  {
      super(getCallback());
  }

  private static Callback getCallback()
  {
      if ( callback.get() == null ) 
          callback.set(new Callback());
      return callback.get();
  }

Единый обратный вызов для всех потоков:

  private final static Callback callback;

  static {
      callback = new Callback(); 
  }

  public Foo()
  {
      super(getCallback());
  }

  private static Callback getCallback()
  {
      return callback;
  }

И, для полноты, по одному обратному вызову на объект:

  private Callback callback;

  public Foo()
  {
      super(getCallback());
  }

  private Callback getCallback()
  {
      callback = new Callback();
      return callback;
  }