Подтвердить что ты не робот

Сколько потоковой безопасности слишком много?

Я читал Java Concurrency на практике в последнее время - отличная книга. Если вы считаете, что знаете, как работает Concurrency, но в большинстве случаев вы сталкиваетесь с реальными проблемами, похоже, что SWAG - это самое большее, что вы можете сделать, то эта книга, несомненно, прольет некоторый свет на эту тему. Это страшно, как многие вещи могут пойти не так, когда вы пытаетесь обмениваться данными между потоками. Думаю, это заставило меня, наверное, немного сумасшедшим в отношении безопасности потоков. Теперь я обеспокоен тем, что с чересчур большой синхронизацией я могу столкнуться с некоторыми проблемами с жизнеспособностью. Вот фрагмент кода, иллюстрирующий:

   private final Hashtable<String, AtomicInteger> userSessions =
new Hashtable<String, AtomicInteger>();

   public void registerUser(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount != null) {
               sessionCount.incrementAndGet();
           } else {
               userSessions.put(userLogin, new AtomicInteger(1));
           }
       }
   }

   public void unregisterUser(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount != null) {
               sessionCount.decrementAndGet();
           }
       }
   }

   public boolean isUserRegistered(String userLogin) {
       synchronized(userSessions) {
           AtomicInteger sessionCount = userSessions.get(userLogin);
           if (sessionCount == null) {
               return false;
           }
           return sessionCount.intValue() > 0;
       }
   }

Я попытался все исправить: синхронизированный сборник, построенный в статическом разделе и хранящийся в статической окончательной ссылке для безопасной публикации, блокировка в коллекции (вместо this - чтобы я не блокировал весь класс кодом живет в) и использует классы атомной оболочки для примитивов. В книге упоминается, что переусердство может также вызвать проблемы, но мне кажется, мне нужно больше времени, чтобы полностью обернуть вокруг себя голову. Как бы вы сделали этот код потокобезопасным и удостоверились, что он не страдает от живучести и проблем с производительностью?

EDIT: превратили его в методы и переменные экземпляра, изначально все было объявлено как статический - плохой, плохой дизайн. Также сделал userSessions частным (как-то я оставил его публичным до этого).

4b9b3361

ОТВЕТЫ

Ответ 1

Используйте ConcurrentHashMap, чтобы вы могли использовать putIfAbsent. Для синхронизации не требуется AtomicInteger код.

   public final ConcurrentMap<String, AtomicInteger> userSessions =
       new ConcurrentHashMap<String, AtomicInteger>();

   public void registerUser(String userLogin) {
       AtomicInteger newCount = new AtomicInteger(1);
       AtomicInteger oldCount = userSessions.putIfAbsent(userLogin, newCount);
       if (oldCount != null) {
           oldCount.incrementAndGet();
       }
   }

   public void unregisterUser(String userLogin) {
       AtomicInteger sessionCount = userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount.decrementAndGet();
       }
   }

   public boolean isUserRegistered(String userLogin) {
       AtomicInteger sessionCount = userSessions.get(userLogin);
       return sessionCount != null && sessionCount.intValue() > 0;
   }

Обратите внимание, что это утечка...

Попытка выполнить негерметичную версию:

   public final ConcurrentMap<String, Integer> userSessions =
       new ConcurrentHashMap<String, Integer>();

   public void registerUser(String userLogin) {
       for (;;) {
           Integer old = userSessions.get(userLogin);
           if (userSessions.replace(userLogin, old, old==null ? 1 : (old+1)) {
                break;
           }
       }
   }
   public void unregisterUser(String userLogin) {
       for (;;) {
           Integer old = userSessions.get(userLogin);
           if (old == null) {
               // Wasn't registered - nothing to do.
               break;
           } else if (old == 1) {
               // Last one - attempt removal.
               if (userSessions.remove(userLogin, old)) {
                   break;
               }
           } else {
               // Many - attempt decrement.
               if (userSessions.replace(userLogin, old, old-1) {
                   break;
               } 
           }
       }
   }
   public boolean isUserRegistered(String userLogin) {serLogin);
       return userSessions.containsKey(userLogin);
   }

Ответ 2

Прежде всего: не используйте Hashtable! Он старый и очень медленный.
Дополнительно: Синхронизация на более низком уровне не требуется, если вы уже синхронизировались на более высоком уровне (это верно и для объекта AtomicInteger).

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

Подход чтения/записи

Предполагая, что вы вызываете метод isUserRegistered очень часто, а другие методы - только время от времени, хорошим способом является блокировка чтения-записи: ему разрешено одновременно иметь несколько чтений, но только одно приложение write- блокировать, чтобы управлять ими всеми (можно получить, только если никакой другой блокировки не будет получен).

private static final Map<String, Integer> _userSessions =
  new HashMap<String, Integer>();

private ReadWriteLock rwLock =
  new ReentrantReadWriteLock(false); //true for fair locks

public static void registerUser(String userLogin) {
  Lock write = rwLock.writeLock();
  write.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount = Integer.valueOf(sessionCount.inValue()+1);
       } else {
           sessionCount = Integer.valueOf(1)
       }
       _userSessions.put(userLogin, sessionCount);
   } finally {
     write.unlock();
   }
}

public static void unregisterUser(String userLogin) {
  Lock write = rwLock.writeLock();
  write.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           sessionCount = Integer.valueOf(sessionCount.inValue()-1);
       } else {
           sessionCount = Integer.valueOf(0)
       }
       _userSessions.put(userLogin, sessionCount);
   } finally {
     write.unlock();
   }
}

public static boolean isUserRegistered(String userLogin) {
  boolean result;

  Lock read = rwLock.readLock();
  read.lock();
  try {
       Integer sessionCount = _userSessions.get(userLogin);
       if (sessionCount != null) {
           result = sessionCount.intValue()>0
       } else {
           result = false;
       }
   } finally {
     read.unlock();
   }

   return false;
}

Pro: просто понять
Con: Не будет масштабироваться, если часто вызывать методы записи

Малый подход к атомной операции

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

public final ConcurrentMap<String, AtomicInteger> userSessions =
   new ConcurrentHashMap<String, AtomicInteger>();
//There are other concurrent Maps for different use cases

public void registerUser(String userLogin) {
  AtomicInteger count;
  if (!userSession.containsKey(userLogin)){
    AtomicInteger newCount = new AtomicInteger(0);
    count = userSessions.putIfAbsent(userLogin, newCount);
    if (count == null){
      count=newCount;
    }
    //We need ifAbsent here, because another thread could have added it in the meantime
  } else {
    count = userSessions.get(userLogin);
  }
  count.incrementAndGet();
}

public void unregisterUser(String userLogin) {
  AtomicInteger sessionCount = userSessions.get(userLogin);
  if (sessionCount != null) {
    sessionCount.decrementAndGet();
  }
}

public boolean isUserRegistered(String userLogin) {
  AtomicInteger sessionCount = userSessions.get(userLogin);
  return sessionCount != null && sessionCount.intValue() > 0;
}

Pro: очень хорошо масштабируется
Con: Неинтуитивно, будет сложным быстро, не всегда возможно, много скрытых ловушек

Замок для каждого пользователя

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

Ответ 3

С вашего кода ваша синхронизация на _userSessions должна быть достаточной, потому что вы не открываете объекты AtomicInteger.

Добавленная безопасность, предлагаемая AtomicInteger, в этом случае не нужна, поэтому в основном вы используете ее здесь как изменяемый Integer. Вы можете поместить вложенный статический класс, содержащий счет сеанса, только как атрибут на карте, если вы беспокоитесь о дополнительных накладных расходах в AtomicInteger (или немного уродливее: добавьте int [1] на карту, если они не являются выставлены вне этого класса.)

Ответ 4

Хорошая книга, я недавно прочитал ее сам.

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

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

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

Класс A блокирует ресурсы, а затем вызывает B (может быть так же просто, как get/set), который также блокирует ресурс. Другой поток вызывает B, который блокирует ресурсы, а затем вызывает A, вызывающий тупик.

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

Ответ 5

Я столкнулся с этим многолетним вопросом, ища совет о том, как сделать то, что можно назвать "параллельной подсчетной картой" - поиск, в частности, для использования ConcurrentHashMap с AtomicInteger.

Здесь приведена измененная версия ответа с наивысшим рейтингом, который использует AtomicInteger и не течет. В моем (ограниченном) тестировании это выглядит намного быстрее, чем версия Integer. Я также отмечу, что использование ConcurrentMap.get() до ConcurrentMap.putIfAbsent(), похоже, сохраняет заметное время.

private final ConcurrentMap<String, AtomicInteger> userSessions =
   new ConcurrentHashMap<String, AtomicInteger>();

public void registerUser(String userLogin) {
    AtomicInteger oldCount = userSessions.get(key);
    if(oldCount!=null && getAndIncrementIfNonZero(oldCount)>0) return;
    AtomicInteger newCount = new AtomicInteger(1);
    while(true) {
        oldCount = userSessions.putIfAbsent(key, newCount);
        if(oldCount==null) return;
        if(getAndIncrementIfNonZero(oldCount)>0) return;
    }
}

public void unregisterUser(String userLogin) {
   AtomicInteger sessionCount = userSessions.get(userLogin);
   if (sessionCount != null) {
       int endingCount = sessionCount.decrementAndGet();
       if(endingCount==0) userSessions.remove(userLogin);
   }
}

public boolean isUserRegistered(String userLogin) {
   AtomicInteger sessionCount = userSessions.get(userLogin);
   return sessionCount != null && sessionCount.intValue() > 0;
}

private static int getAndIncrementIfNonZero(AtomicInteger ai) {
    while(true) {
        int current = ai.get();
        if(current==0) return 0;
        if(ai.compareAndSet(current, current+1)) return current;
    }
}

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