Является ли Map of Locks безопасным подходом для одновременных операций?

Question

Является ли Map of Locks безопасным подходом для одновременных операций?

Требование состоит в том, что только одному потоку должно быть разрешено выполнять операции управления пользователем (создание / обновление / импорт), но нескольким потокам не разрешено выполнять операции пользователя одновременно для одного и того же пользователя. Например, когда поток A создает пользователя A, одновременно потоку B не должно быть разрешено импортировать пользователя A или создавать пользователя A, но потоку B разрешено импортировать пользователя B. Безопасен ли приведенный ниже поток кода для этих требований?

public class UserManagement {

    ConcurrentHashMap<Integer, Lock> userLock = new ConcurrentHashMap<>();

    public void createUser(User user, Integer userId) {
        Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
        try {
            lock.lock();
            //create user logic
        } finally {
            lock.unlock();
        }
    }

    public void importUser(User user, Integer userId) {
        Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
        try {
            lock.lock();
            //import user logic
        } finally {
            lock.unlock();
        }
    }

    public void updateUser(User user, Integer userId) {
        Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());
        try {
            lock.lock();
            // update user logic
        } finally {
            lock.unlock();
        }
    }
}

4

java multithreading locking

Источник

user1580974 12 авг '16 в 06:00

4 ответа

Решение

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

Чтобы сделать ваш класс полностью потокобезопасным, вам нужно добавить final модификатор к вашему userLockВ этом случае Java Memory Model гарантирует правильную инициализацию этого поля в многопоточной среде. Это также хорошая практика, чтобы сделать неизменяемые поля окончательными.

Важное обновление: @sdotdi отметил в комментариях, что после того, как конструктор завершит свою работу, вы можете полностью положиться на внутреннее состояние объекта. На самом деле, это не так и все сложнее.

Ссылка, приведенная в комментарии, охватывает только раннюю стадию компиляции кода Java и ничего не говорит о том, что будет дальше. Но, кроме того, оптимизация начинает играть и начинает переупорядочивать инструкции по своему усмотрению. Единственное ограничение, которое имеет оптимизатор кода - это JMM. И в соответствии с JMM, вполне законно изменить назначение указателя на новый экземпляр объекта в соответствии с тем, что произошло в его конструкторе. Итак, ничто не мешает оптимизировать этот псевдокод:

UserManagement _m = allocateNewUserManagement(); // internal variable
_m.userLock = new ConcurrentHashMap<>();
// constructor exits here
UserManagement userManagement = _m;  // original userManagement = new UserManagement()

к этому:

UserManagement _m = allocateNewUserManagement();
UserManagement userManagement = _m;
// weird things might happen here in a multi-thread app
// if you share userManagement between threads
_m.userLock = new ConcurrentHashMap<>();

Если вы хотите предотвратить такое поведение, вам нужно использовать какую-то синхронизацию: synchronized, volatile или мягче final как в этом случае.

Более подробную информацию вы можете найти, например, в книге "Параллелизм Java на практике", раздел 3.5 "Безопасная публикация".

5

Источник

user225703 12 авг '16 в 06:37

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

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

Еще один минор с логикой приложения. При передаче пользователя необходимо убедиться, что вы не передаете одного и того же пользователя с разными идентификаторами пользователя (не уверен, почему вы передаете идентификатор пользователя отдельно от объекта пользователя). Это требует дополнительной логики вне этого класса для создания / импорта нового пользователя. В противном случае вы можете в итоге вызвать createUser(userA, 1) и createUser(userA,2) или import(userA,3).

1

Источник

user4828220 12 авг '16 в 06:16

Есть некоторые проблемы:

карта увеличивается до бесконечности
блокировка не должна вызываться внутри try-final
не создавать объект, если существующий ключ

Теперь исправить первую проблему не просто - просто позвонив userLock.remove(userId); недостаточно:

public class UserManagement {

    private final ConcurrentHashMap<Integer, Lock> userLock = new ConcurrentHashMap<>();

    public void createUser(User user, Integer userId) {
        Lock lock = userLock.computeIfAbsent(userId, k -> new ReentrantLock());
        lock.lock();
        try {
            // do user logic
        } finally {
            lock.unlock();
        }
        // Danger: this could remove the lock although another thread is still inside the 'user logic'
        userLock.remove(userId);
    }
}

Насколько мне известно, вы можете исправить все проблемы, сэкономить даже немного памяти и избежать явной блокировки. Согласно Javadocs, единственным требованием является быстрая "пользовательская логика":

// null is forbidden so use the key also as the value to avoid creating additional objects
private final ConcurrentHashMap<Integer, Integer> userLock = ...;

public void createUser(User user, Integer userId) {
  // Call compute if existing or absent key but do so atomically:
  userLock.compute(userId, (key, value) -> {
      // do user logic
      return key;
  });
  userLock.remove(rowIndex);
}

0

Источник

user194609 24 сен '16 в 13:40

Другие вопросы по тегам java multithreading locking

user801894 12 авг '16 в 10:05 2016-08-12 10:05 · Accepted Answer · 2016-08-12 10:05

В вашей программе есть еще одна ошибка, кроме той, о которой говорил Андрей Лыгин.

Это устанавливает lock в null если userId ранее не было замечено, потому что `putIfAbsent (...) не возвращает новое значение, оно возвращает предыдущее значение:

Lock lock = userLock.putIfAbsent(userId, new ReentrantLock());

Сделайте это вместо этого:

Lock lock = userLock.computeIfAbsent(userId, k -> new ReentrantLock());

computeIfAbsent(...) возвращает новое значение. Кроме того, у него есть побочное преимущество: он не создает новый объект Lock, если только он не нужен. (Престижность @bowmore для того, чтобы предложить это.)

Эта программа безопасна?

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

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

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

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