Имеется таск. Нужно реализовать многопоточное приложение, без использования synchronized.
Есть порт с причалами(1 причал - 1 корабль). Есть корабли с грузом и склад. В причале корабль может перенести груз в склад либо на другой корабль.
Я пробовал просто в каждой функции ставить в начале и конце ReentrantLock. Видимо, я плохо понял как он работает. С synchronized можно просто навесить его на каждый трэд, который первым приходит в функцию и всё работает на ура, а без него - тупик. Может кто подсказать как же правильно использовать lock или другой способ реализации такого приложения?
GitHub
Кусок кода:
private ReentrantLock lock = new ReentrantLock();
public boolean tryTransfer(Ship ship) {// ф-ия пытается добавить груз(cargo) в хранилище
boolean flag; // capacity - объём хранилища
lock.lock();
if (capacity + ship.getCargo()>maxCapacity) {
flag = false;
} else
{
capacity+=ship.getCargo();
ship.setCargo(0);
flag = true;
}
lock.unlock();
return flag;
}
ReentrantLock внутри методов и там же их используете (это видно в классах Dock и Ship). Это не имеет никакого смысла, если эти Lock'и вы не передаёте куда-либо ещё. Храните ReentrantLock в полях.Ваша реализация Storage#tryTransfer (та, что на GitHub, а не здесь) почти корректная. Вы правильно ограничиваете доступ к данным самого Storage. Однако в этом методе вы неправильно работаете с Ship - методы Ship#getCargo и Ship#setCargo не являются синхронизированными.
В данном случае вам нужно брать блокировку и в Storage, и Ship. Примерно так (по аналогии можете переделать и всё остальное):
public class Ship {
private final ReentrantLock lock = new ReentrantLock();
private double cargo;
public double getCargo() {
this.lock.lock();
try {
return this.cargo;
} finally {
this.lock.unlock();
}
}
public void setCargo(double cargo) {
this.lock.lock();
try {
this.cargo = cargo;
} finally {
this.lock.unlock();
}
}
public ReentrantLock getLock() {
return this.lock;
}
}
public class Storage {
private final ReentrantLock lock = new ReentrantLock();
private final double maxCapacity = 100;
private double capacity;
public boolean tryTransfer(Ship ship) {
this.lock.lock();
try {
ship.getLock().lock();
try {
if (this.capacity + ship.getCargo() > this.maxCapacity) {
return false;
}
this.capacity += ship.getCargo();
ship.setCargo(0);
} finally {
ship.getLock().unlock();
}
return true;
} catch (Exception e) {
e.printStackTrace();
} finally {
this.lock.unlock();
}
return false;
}
}
Зачем в пункте #2 в Storage#tryTransfer я дополнительно брал блокировку над Ship#getLock, если она уже была в Ship#getCargo и Ship#setCargo? Дело в том, что возможна ситуация, когда одновременно несколько потоков будут работать с одним и тем же экземпляром Ship. Т.е. мы вызвали Ship#getCargo и получили, например, 10. И ожидаем, что, когда мы вызовем Ship.setCargo(0), cargo всё ещё будет равен 10. Однако без дополнительной синхронизации между нашими вызовами Ship#getCargo и Ship#setCargo какой-нибудь другой поток может сам вызвать Ship#setCargo с каким-нибудь другим значением, и получится, что в момент нашего вызова Ship#setCargo(0) cargo равен не 10, как было изначально, а например, 12.
UPDATE
Ship#run вы забываете взять блокировку у Dock при работе с ним.Ship#run вы вызываете ReentrantLock#lock внутри try-finally, а не до него. Так делать нельзя, так как есть риск, что try-finally прервётся (например, из-за исключения) до взятия блокировки, а ReentrantLock#unlock всё равно будет вызван.Ship#run вы вызываете Dock#setCurrentShip? Не лучше просто положить Ship в Dock#shipsQueue, а дальше причал сам разберётся, какой корабль обработать в первую очередь?Dock#run после вызова ReentrantLock#lock сразу делайте try-finally. Исключение может произойти даже в обычном System.out.println(), из-за чего ваш ReentrantLock#unlock не будет вызван.Dock#run вы сперва проверяете Dock#currentShip и только потом берёте блокировку. И то не всегда. Если бы Dock#currentShip не торчал наружу, то это было бы нормально, но ведь у вас есть Dock#getCurrentShip и Dock#setCurrentShip, что означает необходимость их синхронизации.Dock#setShipsQueue. Не дело, когда кто угодно может взять и просто так подменить столь важную коллекцию. Лучше вообще её создавать внутри Dock (а не принимать снаружи) и никому не отдавать - просто добавить публичные Dock#addShip и Dock#removeShip. Ну и Dock#isFree можно модифицировать, чтобы он не значение какого-то поля возвращал, а проверял Dock#currentShip и Dock#shipsQueue (хотя тут уже решайте сами - ваша изначальная задумка мне неизвестна).Dock#isFree всегда будет возвращать false после первой обработки любого непустого корабля. Мне кажется, тут что-то не так - корабли когда-нибудь закончатся => причал должен в таком случае стать пустым, чего не происходит.UPDATE
ReentrantLock#unlock не нужно вызывать ReentrantLock#tryLock. Поток уже владеет блокировкой, так что такая проверка ничего не даст.Ship#run у вас происходит бесконечное добавление корабля в очередь. Просто уберите цикл, он не нужен.Storage#run совершенно некорректна. Даже если опустить пункт #1, вы зачем-то освобождаете блокировки перед return и тому подобными операторами. В этом нет никакой необходимости - блок finally выполняется всегда, даже после return или continue.Продвижение своими сайтами как стратегия роста и независимости