Моя реализация долго выполняющейся задачи с прогрессом и ее слабые места

#c_sharp #aspnet #aspnet_mvc #многопоточность #инспекция_кода

Ситуация: имеется приложение asp.net mvc. Есть кнопка по нажатии на которую запускается
процесс генерации некоего отчета. Отчет может генерироваться довольно долго например
несколько минут. Необходимо также после запуска генерации показать пользователю модальное
окно с прогрессом выполнения генерации и возможностью отмены. Я реализовал это так. 

private static readonly IList DownloadStates = new List();

public ActionResult Export()
{           
    var task = new DownloadTask(Guid.NewGuid());
    DownloadStates.Add(task);

    string zipFilename = string.Format("{0}.zip", task.Id);
    GenerateAsync(zipFilename, task);

    return Json(task.Id, JsonRequestBehavior.AllowGet);
}


Этот экшен вызывается при нажатии на кнопку генерации отчета. Тут происходит вот
что. Сначала создается задача task. Код этого класса выглядит так 

public class DownloadTask
{
    public DownloadTask() { }

    public DownloadTask(Guid id)
    {
        Id = id;
        State = DownloadState.NotStarted;
        CancellationTokenSource = new CancellationTokenSource();
    }

    public Guid Id { get; set; }

    public double CurrentProgress { get; set; }

    // состояние задачи - запущена, отменена, ошибка, завершена и тд 
    public DownloadState State { get; set; } 
}


Задача помещается в коллекцию DownloadStates. 

Вот так выглядит код асинхронного метода GenerateAsync

public async Task GenerateAsync(string filename, DownloadTask currentTask)
{   
    await Task.Factory.StartNew(() =>
    {
        try
        {
            currentTask.State = DownloadState.Processing;

            foreach (var page in pages)
            {
                // обновление прогресса
                currentTask.CurrentProgress = // ...
                // обработка данных
            }
            currentTask.State = DownloadState.Completed;
        }       
        catch
        {
            currentTask.State = DownloadState.Faulted;
            // обработка ошибки
        }
    }).ConfigureAwait(false);
    return currentTask.Id;
}


То есть нажав на кнопку мы отправляем запрос на генерацию отчета. Создается задача,
ей присваивается уникальный Guid, задача помещается в коллекцию задач DownloadStates
и после этого запускается асинхронный метод GenerateAsync. После запуска этого метода
сервер отправляет в виде ответа клиенту guid созданной задачи. Асинхронный метод что
важно запускается без await по принципу fire and forget. Дальнейшая связь с ним процессом
генерации происходит через коллекцию DownloadStates. 

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

public ActionResult Progress(Guid taskId)
{
    var task = DownloadStates.FirstOrDefault(x => x.Id == taskId);
    string state = "";
    bool isFaulted = false;
    bool isCancelled = false;

    if (task != null)
    {
        switch (task.State)
        {                           
            case DownloadState.Faulted:
                isFaulted = true;
                DownloadStates.Remove(task);
                break;
            case DownloadState.Processing:
                state = "processing";
                break;
            // ... прочие ветки     
        }
    }
    else
        isFaulted = true;

    return Json(new
    {
        progress = task != null && task.State != DownloadState.Completed ? task.CurrentProgress
* 100 : 100,
        text = state, 
        isFaulted,
        isCancelled
    }, JsonRequestBehavior.AllowGet);
}


Здесь происходит следующее: если задача выполняется то в ответе возвращаем клиенту
текущий прогресс (он хранится в таске в коллекции DownloadStates а обновляется внутри
асинхронного метода). Если во время выполнения асинхронного метода произошло какое-то
исключение, то клиенту возвращается isFaulted = true и клиент получив это значение
останавливает таймер и прекращает слать запросы о прогрессе (задача завершилась с ошибкой)
и отображает сообщение об ошибке. Если же задача имеет статус Completed то прогресс
устанавливается в 100% и таймер на клиенте также останавливается после чего выполняется
запрос на загрузку сформированного отчета. 

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

Хотелось бы услышать мнение более опытных программистов насчет потенциальных проблем
моего кода (утечки памяти, незавершенные таски, необработанные исключения, может быть
где-то надо было поставить lock или могут возникнуть блокировки или что-то еще?). Есть
ли тут какие-то слабые места (а они я уверен есть), что тут стоило бы улучшить особенно
с учетом многопоточного выполнения и потенциального запуска нескольких задач одновременно.
Заранее спасибо!

P.S. к сожалению использование SignalR оказалось невозможным по независящим от меня
причинам поэтому вариант переделать все на SignalR я рассматривать не могу
    

Ответы

Ответ 1

Основные проблемы


Нет lock на обращениях к DownloadStates. Класс List не потокобезопасен, доступ к
нему нужно вручную синхронизировать.
Task.Factory.StartNew не обязательно запускает таск в новом потоке. Вместо него стоит
использоватьTask.Factory.Run.
Единственный более-менее надежный метод выполнения фоновых задач в ASP.NET - это
QueueBackgroundWorkItem.
Ваш подход не слишком применим в живых приложениях, при наличии двух и более web-серверов,
и, соответственно, двух копий приложения.


Гораздо надежнее не запускать фоновые задачи в самом ASP.NET, а положить задания
в базу данных, и сделать win-сервис, который будет их оттуда доставать, выполнять,
 и отчитываться через базу о прогрессе.



Подробнее:

Синхронизация списков

List работает на простых массивах внутри. Т.е. поиск FirstOrDefault в нем сделан
обычным перебором элементов по индексу от 0 до N-1. 

Предположим у вас запустилось два таска - 0 и 1. 


Таск 0 выполнился. 
Пришел запрос на Progress для Task 0. Дошел до строчки с Remove
Пришел запрос на Progress для Task 1. Зашел в поиск FirstOrDefault, дошел до индекса 1
Запрос на Progress для Task 0 взял и удалил свой элемент из списка!
В Progress для Task 1 вызов FirstOrDefault... падает? возвращает null?


QueueBackgroundWorkItem

В ASP.NET есть механизм рейсайкла апппула. В определенный момент IIS может взять
и перезапустить процесс, в котором выполняется приложение. По умолчанию это происходит
раз в 29 часов. Может происходить по лимиту памяти. Или по неактивности. Обычный таск
из Thread Pool или любой другой фоновый поток будет просто прибит при ресайкле, мгновенно
и без каких-либо уведомлений.

Таск, созданный через QueueBackgroundWorkItem, получит уведомление о выключении (через
CancellationToken). Кроме того, у него будет 90 секунд чтобы на это уведомление отреагировать.
Не сверхнадежно, но лучше чем ничего.

Проблемы с двумя Web-серверами

Если у вас два бэкенда - то у вас две независимых копии приложения. Два списка DownloadStates.
Если запрос о старте таска пришел на один сервер, а запрос прогресса - на другой -
то пользователь получит isFaulted. 

Решается переносом очереди закачек в базу или в любое другое общее хранилище (например,
сервис с очередью).