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

#java #concurrency

Подскажите, пожалуйста, является ли 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 обеспечивает потокобезопасность только атомарных
операций, т.е. еще необходима дополнительная синхронизация содержимого. А не может
ли в такой ситуации возникнуть дедлока?
    

Ответы

Ответ 1

В этом коде много проблем с потокобезопасностью, хотя многие вещи зависят от того,
как он используется. Например, 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 синхронизация не нужна.