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

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

3 голосов
/ 01 июня 2009 
private static Callback callback;

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

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

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

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

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

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

РЕДАКТИРОВАТЬ: я намерен оставить «обратный вызов» где-то в классе, чтобы я мог использовать его позже. Но это нелегко, потому что Foo происходит от класса, в котором есть конструктор, передающий 'callback' для передачи.

Ответы [ 7 ]

6 голосов
/ 01 июня 2009

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

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

5 голосов
/ 01 июня 2009

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

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

private static final Callback callback = new Callback();

public Foo() {
    super(callback);
}

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

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

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

3 голосов
/ 01 июня 2009

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

2 голосов
/ 01 июня 2009

Я знаю, что на него ответили, но почему не было подробно описано.

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

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

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

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

Надеюсь, это поможет.

2 голосов
/ 01 июня 2009

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

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

1 голос
/ 02 июня 2009

Вы хотите один обратный вызов на поток, один на объект или истинный синглтон?

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

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

Один на тему: 

  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;
  }
1 голос
/ 02 июня 2009

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

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());
}

Обе реализации являются поточно-ориентированными.

...