Проверка сервиса на thread-safe

Подскажите, пожалуйста, является ли thread-safe следующая реализация сервиса.
public class TaskReport {
private final String taskID; private final Date startTime = new Date(); private Date endTime = null; private final List report = new ArrayList(); private volatile boolean completed = false;
public TaskReport(String taskID) { this.taskID = taskID; }
public String getTaskID() { return taskID; }
public Date getStartTime() { return startTime; }
public Date getEndTime() { synchronized ( endTime ) { return endTime; } }
public void append(String entry) { synchronized ( report ) { report.add(entry); } }
public List getReport() { synchronized ( report ) { return report; } }
public boolean isCompleted() { return completed; }
public void setCompleted() { this.completed = true; synchronized ( endTime ) { this.endTime = new Date(); } }
}
public class TaskReportService {
private final ConcurrentHashMap tasks = new ConcurrentHashMap<>();
public TaskReport get(String key) { return tasks.get(key); }
public void create(TaskReport report) { tasks.putIfAbsent(report.getTaskID(), report); }
public void delete(String key) { tasks.remove(key); }
}
Насколько я понимаю ConcurrentHashMap обеспечивает потокобезопасность только атомарных операций, т.е. еще необходима дополнительная синхронизация содержимого. А не может ли в такой ситуации возникнуть дедлока?

Ответ

В этом коде много проблем с потокобезопасностью, хотя многие вещи зависят от того, как он используется. Например, TaskReportService.create
public void create(TaskReport report) { tasks.putIfAbsent(report.getTaskID(), report); }
Вас точно устраивает ситуация, что созданный снаружи кем-то TaskReport может быть помещён в map, а может и не быть? Причём вы никак не узнаете, был ли он помещён. Я могу снаружи сделать:
TaskReport report = new TaskReport("foo"); service.create(report); report.append("bar");
При этом совершенно неизвестно, сохранилась ли строчка "bar" или нет. Допустим вы всегда вызываете service.create для свежесозданного TaskReport, а потом его не используете. Зачем тогда его создание выносить наружу, раз это может привести к потенциальным проблемам? Не проще ли сделать метод getOrCreate?
public TaskReport getOrCreate(String key) { TaskReport report = tasks.get(key); if(report != null) return report; // Нету - создаём TaskReport newReport = new TaskReport(key); TaskReport oldReport = tasks.putIfAbsent(key, newReport); if(oldReport != null) { // Ой, кто-то в другом потоке только что создал с тем же ключом вперёд нас // Выкидываем наш новый и возвращаем тот, что есть return oldReport; } // Возвращаем свежесозданный return newReport; }
Теперь если вы будете для создания TaskReport пользоваться только этим методом, у вас гарантировано не будет двух репортов для одного ключа, по крайней мере пока вы не начнёте удалять и создавать с тем же ключом.
Кстати, если у вас Java-8, вышеприведённый метод будет однострочным:
public TaskReport getOrCreate(String key) { return tasks.computeIfAbsent(key, TaskReport::new); }
Далее — класс TaskReport. Тут тоже многое зависит от того, как он используется. Попытки синхронизации подсказывают, что один и тот же TaskReport может использоваться из нескольких потоков одновременно. Вот такой код абсолютно бесполезен:
public Date getEndTime() { synchronized ( endTime ) { return endTime; } }
Вы совершенно ни от чего не защищаетесь. А вот это ещё хуже:
synchronized ( endTime ) { this.endTime = new Date(); }
На это вроде даже FindBugs ругается. Бессмысленно синхронизоваться по объекту в поле и присваивать в это же поле другое значение. Надо сперва понять, от чего вы защищаетесь. Если вы не собираетесь менять содержимое объектов Date (например, вызывая Date.setTime()), то вам нечего защищать. В крайнем случае объявите поле endTime как volatile, чтобы изменения были видны в других потоках сразу. Синхронизация здесь излишня и бесполезна.
Аналогично бесполезно вот это:
synchronized ( report ) { return report; }
Когда вы уже вернёте ArrayList наружу, блок синхронизации завершится и дальше всё зависит от того, что будет делать внешний пользователь с этим списком. Если он собрался обходить его итератором (например, чтобы вывести строки в интерфейсе пользователя), вас не спасает ни синхронизация в append, ни в этом методе. Вы можете словить ConcurrentModificationException или чего похуже. Вас не спасёт даже оборачивачивание списка в Collections.synchronizedList. Здесь я вижу три варианта:
Если вы часто пишете и редко читаете, проще вернуть копию:
public List getReport() { synchronized ( report ) { return new ArrayList<>(report); } } Если вы редко пишете и часто читаете, можно воспользоваться CopyOnWriteArrayList
private final List report = new CopyOnWriteArrayList();
В этом случае синхронизация не нужна вообще (ни в append, ни в геттере). Просто верните report в геттере. Можно обернуть в Collections.unmodifiableList для безопасности. Если вы часто пишете и часто читаете, но вам не нужен случайный доступ, воспользуйтесь ConcurrentLinkedDeque:
private final Collection report = new ConcurrentLinkedDeque();
public Collection getReport() { return Collections.unmodifiableCollection(report); }
В этом случае тоже в append синхронизация не нужна.