Тестовое задание для Связного FixedThreadPool на C#. Что здесь не так? UPD

UPDATE: Я не мог отказать себе в удовольствии исправить свой ошибочный код. Добавил раздел «Работа над ошибками», в котором привожу исправленный код и описание исправлений, основанное на полученных комментариях.

Это скорее пост-вопрос к специалистам, нежели просто кусок полезной информации. Приглашаю к дискуссии.
Недавно я имел счастье послать своё резюме в Связной на позицию .NET разработчика. В ответ меня попросили сделать тестовое задание на знание многопоточности. Я не могу назвать себя экспертом в этой области, но, тем не менее, прекрасно понял, как мне показалось, как реализовать следующие требования:

Требуется реализация класса на языке C#, аналогичного FixedThreadPool в Java, со следующими требованиями:
  • В конструктор этого класса должно передаваться количество потоков, которые будут выполнять задачи.
  • Интерфейс класса должен предоставлять методы: boolean execute(Task task, Priority priority) и void stop()
  • Интерфейс Task должен содержать один метод: void execute(), который вызывается в произвольном потоке.
  • Тип Priority — это перечисление из трёх приоритетов: HIGH, NORMAL, LOW. При этом во время выбора следующего задания из очереди действуют такие правила: на три задачи с приоритетом HIGH выполняется одна задача с приоритетом NORMAL, задачи с приоритетом LOW не выполняются, пока в очереди есть хоть одна задача с другим приоритетом.
  • До вызова метода stop() задачи ставятся в очередь на выполнение и метод boolean execute(Task task, Priority priority) сразу же возвращает true, не дожидаясь завершения выполнения задачи; а после вызова stop() новые задачи не добавляются в очередь на выполнение, и метод boolean execute(Task task, Priority priority) сразу же возвращает false.
  • Метод stop() ожидает завершения всех текущих задач (не очищая очередь).


Поскольку в задании не было сказано какими примитивами я должен пользоваться, должен ли сделать всё на простейших Thread или же использовать ThreadPool, TPL и т.п., я решил, что задание предполагает использование самых базовых элементов: Thread, ManualResetEvents и т.п. Написал за несколько часов, отослал. Сегодня позвонил и получил ответ через кадровика, который звучал примерно так: «это даже не близко к том, что надо». Это меня озадачило, ибо код работает и протестирован, явных огрехов, на мой взгляд нету.

Итак, на ваш суд представляю мою реализацию FixedThreadPool и сопутствующих классов. Сразу предупреждаю, что, по их мнению реализация ошибочна, и, соответственно, брать мою идею за основу не стоит. Некоторые коментарии по коду:

  • я решил инкапсулировать потоки задач в самом классе задачи,
  • два параметра с типом ILog нужны только для тестовых целей, к основной функциональности они, понятное дело, отношения не имеют,
  • весь проект, включая тестовое приложение можно загрузкить по ссылке (27 килобайт): тестовый проект на ifolder


FixedThreadPool
    /// <summary>
    /// Пул потоков, выполняющий одновременно не более определённого количество задач с учётом их
    /// приоритетов.
    /// </summary>
    /// <remarks>
    /// Тестовый проект для компании Связной.
    /// </remarks>
    public sealed class FixedThreadPool
    {
        #region Constructors

        /// <summary>
        /// Инициализирует новый экземпляр пул потоков максимальным количеством одновременно
        /// выполняемых задач.
        /// </summary>
        /// <param name="aConcurrentTaskNumber">
        /// Максимальное количество одновременно выполняемых задач.
        /// </param>
        /// <param name="aLog">
        /// Сервис логгирования.
        /// </param>
        /// <param name="aPriorityLog">
        /// Лог для вывода приоритета отобранной для выполнения задачи.
        /// </param>
        /// <exception cref="ArgumentOutOfRangeException">
        /// Неправильно заданное максимальное количество одновременно выполняемых задач.
        /// </exception>
        public FixedThreadPool
            (int aConcurrentTaskNumber, ILog aLog = null, ILog aPriorityLog = null)
        {
            if (aConcurrentTaskNumber <= 1)
            {
                throw new ArgumentOutOfRangeException(
                    "aConcurrentTaskNumber",
                    "Количество одновременно выполняемых задач должно быть больше единицы.");
            }

            Log = aLog;
            PriorityLog = aPriorityLog;

            mConcurrentTaskNumber = aConcurrentTaskNumber;

            LogMessage("Создан поток планировщика.");
            Thread lTaskSchedulerThread =
                new Thread(TaskSchedulerLogic) {Name = "Поток планировщика."};
            lTaskSchedulerThread.Start();
            LogMessage("Поток планировщика запущен.");
        }

        #endregion

        #region Public methods

        /// <summary>
        /// Ставит задачу <paramref name="aTask"/> в очередь на выполнение с приоритетом 
        /// <paramref name="aTaskPriority"/>.
        /// </summary>
        /// <param name="aTask">
        /// Задача для постановки в очередь на выполнения..
        /// </param>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        /// <returns>
        /// <see langword="true"/> - задача поставлена в очередь на выполнение. 
        /// <see langword="false"/> - задача не была поставлена в очередь на выполнение, так как
        /// работа пула потока была остановлена.
        /// </returns>
        /// <exception cref="ArgumentNullException">
        /// Задача для постановки в очередь на выполнения не задана.
        /// </exception>
        public bool Execute(Task aTask, TaskPriority aTaskPriority)
        {
            if (aTask == null)
            {
                throw new ArgumentNullException(
                    "aTask", "Задача для постановки в очередь на выполнения не задана.");
            }

            LogMessage("Получена новая задача для выполнения.");
            lock (mIsStoppedLock)
            {
                if (IsStopped)
                {
                    // Запрошена остановка.
                    LogPriority(aTaskPriority, ConsoleColor.DarkGray);
                    // Отклонять новые задачи.
                    return false;
                }
            }

            // Добавить задачу в очередь.
            EnqueueTask(aTask, aTaskPriority);
            LogMessage("Задача добавлена в очередь задач.", ConsoleColor.DarkYellow);

            return true;
        }

        /// <summary>
        /// Останавливает добавлении задач в очередь пула потоков, очередь не очищается. Возвращает
        /// выполнение только после окончания всех имеющихся задач в очереди.
        /// </summary>
        /// <remarks>
        /// После вызова этого метода дальнейшее добавление задач в очередь на выполнение не
        /// возможно и метод <see cref="Execute"/> будет возвращать <see langword="false"/>.
        /// Имеющиеся, на момент выполнения этого метода, задачи в очереди будут выполнены.
        /// </remarks>
        public void Stop()
        {
            // Выставить признак окончания работы пула.
            lock (mIsStoppedLock)
            {
                IsStopped = true;
                LogMessage("Запрошена остановка пула.");
            }

            // Дождаться окончания выполнения всех задач, оставшихся в очереди.
            LogMessage(
                "Начато ожидание завершения выполнения всех оставшихся задач в очереди.",
                ConsoleColor.DarkRed);
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }

            mPoolStoppedGate.WaitOne();

            LogMessage("Дождались окончания выполнения всех задач в очереди.", ConsoleColor.DarkRed);
        }

        #endregion

        #region Private properties

        /// <summary>
        /// Получает/устанавливает признак того, остановлена ли работа пула.
        /// </summary>
        /// <value>
        /// <see langword="true"/> - работа пула остановлена, дальнейшее добавление задач в очередь
        /// не возможно. <see langword="false"/> - работа пула продолжается.
        /// </value>
        /// <remarks>
        /// Объект синхронизации доступа <see cref="mIsStoppedLock"/>.
        /// </remarks>
        private bool IsStopped { get; set; }

        /// <summary>
        /// Получает/устанавливает сервис логгирования приоритета задачи.
        /// </summary>
        private ILog PriorityLog { get; set; }

        /// <summary>
        /// Получает/устанавливает сервис логгирования сообщений.
        /// </summary>
        private ILog Log { get; set; }

        #endregion

        #region Private methods

        /// <summary>
        /// Логика работы планировщика выполнения задач. Выполняется в отдельном потоке.
        /// </summary>
        private void TaskSchedulerLogic()
        {
            lock (mTaskSchedulerLock)
            {
                while (true)
                {
                    // Отпускаем монитор и ждём сигнала, поданного через mTaskSchedulerLock.
                    Monitor.Wait(mTaskSchedulerLock);
                    lock (mQueuedTasksLock)
                    {
                        lock (mIsStoppedLock)
                        {
                            lock (mRunningTasksLock)
                            {
                                if (IsStopped &&
                                    !mRunningTasks.Any() &&
                                    !mQueuedTasks.Any())
                                {
                                    LogMessage(
                                        "Запрошена остановка пула и больше нет задач в очереди на выполнение.");
                                    LogMessage("Планировщик - Выход из потока планировщика.");
                                    // Сигнализировать об окончании выполнения последней задачи.
                                    mPoolStoppedGate.Set();
                                    return;
                                }
                            }
                        }
                    }

                    lock (mQueuedTasksLock)
                    {
                        if (!mQueuedTasks.Any())
                        {
                            // Очередь задач пуста.
                            // Продолжить ожидание.
                            continue;
                        }
                    }

                    lock (mRunningTasksLock)
                    {
                        if (mRunningTasks.Count >= mConcurrentTaskNumber)
                        {
                            // Список выполняемых задач полон.
                            // Продолжить ожидание.
                            continue;
                        }
                    }

                    LogMessage(
                        "Дождались появления задачи в очереди задач.",
                        ConsoleColor.DarkRed);
                    // Дождаться и получить следующую задачу для выполнения.
                    TaskListEntry lTask = DequeueTask();
                    LogMessage("Планировщик - Получена новая задача для выполнения.");
                    // Подписаться на событие завершения выполнения задачи.
                    lTask.Task.Finished += OnTaskFinished;
                    // Добавить задачу в список выполняемых задач.
                    lock (mRunningTasksLock)
                    {
                        mRunningTasks.Add(lTask);
                    }
                    if (lTask.TaskPriority ==
                        TaskPriority.High)
                    {
                        // Запущена задача с высоким приоритетом.
                        // Увеличить значение счётчика запущенных задач с высоким приоритетом на единицу.
                        Interlocked.Increment(ref mQueuedHighPriorityTaskCounter);
                    }
                    else if (lTask.TaskPriority ==
                             TaskPriority.Normal)
                    {
                        // Запущена задача с обычным приоритетом.
                        // Уменьшить значение счётчика запущенных задач с высоким приоритетом на HighPriorityTaskFactor.
                        Interlocked.Add(
                            ref mQueuedHighPriorityTaskCounter, -HighPriorityTaskFactor);
                    }

                    // Запустить задачу на выполнение.
                    lTask.Task.Execute();
                    LogMessage(
                        string.Format(
                            "Планировщик - Запущена задача с приоритетом {0}.",
                            lTask.TaskPriority),
                        ConsoleColor.DarkYellow);

                    lock (mRunningTasksLock)
                    {
                        LogMessage(
                            string.Format(
                                "В списке выполняющихся задач {0} задач.",
                                mRunningTasks.Count));
                    }
                }
            }
        }

        /// <summary>
        /// Обрабатывает событие завершения задачи.
        /// </summary>
        /// <param name="aSender">
        /// Задача-источник события.
        /// </param>
        /// <param name="aEventArgs">
        /// Параметры события.
        /// </param>
        private void OnTaskFinished(object aSender, EventArgs aEventArgs)
        {
            Task lSender = aSender as Task;
            Debug.Assert(
                lSender != null, "В параметре aSender задача должна передавать ссылку на себя.");
            // Отписаться от события завершения, чтобы задача могла быть убрана сборщиком мусора.
            lSender.Finished -= OnTaskFinished;
            // Удалить задачу из списка выполняющихся.
            lock (mRunningTasksLock)
            {
                // Найти и удалить закончившуюся задачу из списка выполняющихся задач.
                TaskListEntry lEntry = mRunningTasks.First(aEntry => aEntry.Task == lSender);
                mRunningTasks.Remove(lEntry);
                LogMessage(
                    string.Format("Задача с приоритетом {0} завершена.", lEntry.TaskPriority),
                    ConsoleColor.Red);
            }

            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }
        }

        /// <summary>
        /// Добавляет задачу <paramref name="aTask"/> в очередь задач на выполнение.
        /// </summary>
        /// <param name="aTask">
        /// Задача, добавляемая в очередь задач на выполнение.
        /// </param>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        private void EnqueueTask(Task aTask, TaskPriority aTaskPriority)
        {
            TaskListEntry lEntry = new TaskListEntry(aTask, aTaskPriority);
            LogPriority(aTaskPriority, ConsoleColor.Green);
            lock (mQueuedTasksLock)
            {
                // Добавить задачу в очередь задач на выполнение.
                mQueuedTasks.Add(lEntry);
                LogMessage(
                    string.Format(
                        "В очередь добавлена задача с приоритетом {0}", lEntry.TaskPriority),
                    ConsoleColor.Green);
                // Поднять барьер доступа к очереди задач.
                LogMessage("Поднять барьер доступа к очереди задач.", ConsoleColor.DarkRed);
            }
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }
        }

        /// <summary>
        /// Ожидает появления в очереди задач хотя бы одной задачи. Дождавшись изымает следующую
        /// задачу из очереди, учитывая правила приоритетов.
        /// </summary>
        /// <returns>
        /// Задача, изъятая из очереди задач на выполнение.
        /// </returns>
        private TaskListEntry DequeueTask()
        {
            TaskListEntry lNextTask;
            lock (mQueuedTasksLock)
            {
                lNextTask = FindNextTaskUsingPriorityRules();
                LogPriority(lNextTask.TaskPriority, ConsoleColor.Red);
                LogMessage(
                    string.Format(
                        "Получена задача из очереди задач с приоритетом {0}.",
                        lNextTask.TaskPriority),
                    ConsoleColor.DarkRed);
                mQueuedTasks.Remove(lNextTask);
            }
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }

            return lNextTask;
        }

        /// <summary>
        /// Находит следующую задачу с учётом правил приоритезации.
        /// </summary>
        /// <returns>
        /// Найденная следующая задача для выполнения.
        /// </returns>
        /// <remarks>
        /// ЗАМЕЧАНИЯ ПО ПРАВИЛАМ ПРИОРИТЕЗАЦИИ: Полученные мной правила выбора следующей задачи
        /// определены в задании не полностью. В частности, не было сказано что делать, если в
        /// очереди задач есть только задачи с обычным приоритетом. Также не определено, что нужно
        /// делать, если было выполнено больше, чем три задачи с высоким приоритетом. Поэтому я
        /// додумал эти правила, исходя из предположения, что лучше пусть выполняется хоть что-то,
        /// чем ждать появления задач с каким-то определённым приоритетом.
        /// </remarks>
        private TaskListEntry FindNextTaskUsingPriorityRules()
        {
            TaskListEntry lNextTask;
            lock (mQueuedTasksLock)
            {
                Debug.Assert(
                    mQueuedTasks.Count > 0,
                    "Метод FindNextTaskUsingPriorityRules не должен вызываться, если очередь задач пуста.");
                // По умолчанию будет выполняться задача с высоким приоритетом.
                TaskPriority lNextTaskPriority = TaskPriority.High;
                // Проверить возможность выполнения задачи с более низкими приоритетами.
                if (mQueuedTasks.All(aEntry => aEntry.TaskPriority == TaskPriority.Low))
                {
                    // В очереди задач все задачи с низким приоритетом.
                    // Следующая задача будет с низким приоритетом.
                    lNextTaskPriority = TaskPriority.Low;
                }
                else
                {
                    // Условие для задач с низким приоритетом не выполняется.
                    if (mQueuedTasks.Any(
                        aEntry => aEntry.TaskPriority == TaskPriority.Normal) &&
                        (mQueuedTasks.All(
                            aEntry => aEntry.TaskPriority != TaskPriority.High) ||
                         Interlocked.CompareExchange(ref mQueuedHighPriorityTaskCounter, 0, 0) >=
                         HighPriorityTaskFactor))
                    {
                        // В списке задач на выполнение есть задачи с обычным приоритетом и
                        // выполнено достаточное количество задач с высоким приоритетом или
                        // в очереди задач все задачи имеют приоритет ниже высокого.
                        // Следующая задача будет с обычным приоритетом.
                        lNextTaskPriority = TaskPriority.Normal;
                    }
                }

                lNextTask = mQueuedTasks.First(
                    aEntry => aEntry.TaskPriority == lNextTaskPriority);
            }

            return lNextTask;
        }

        /// <summary>
        /// Выводит сообщение в лог.
        /// </summary>
        /// <param name="aMessage">
        /// Сообщение.
        /// </param>
        /// <param name="aColor">
        /// Цвет сообщения.
        /// </param>
        private void LogMessage(string aMessage, ConsoleColor aColor = ConsoleColor.Yellow)
        {
            if (Log == null)
            {
                return;
            }

            Log.WriteMessage(aMessage, aColor);
        }

        /// <summary>
        /// Выводит приоритет задачи в лог.
        /// </summary>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        /// <param name="aColor">
        /// Цвет сообщения.
        /// </param>
        private void LogPriority(TaskPriority aTaskPriority, ConsoleColor aColor)
        {
            if (PriorityLog == null)
            {
                return;
            }

            string lPriority = aTaskPriority == TaskPriority.High
                                   ? "H"
                                   : aTaskPriority == TaskPriority.Normal ? "N" : "L";
            PriorityLog.WriteMessage(lPriority, aColor);
        }

        #endregion

        #region Private data

        /// <summary>
        /// Количество задач с высоким приоритетом, которое должно быть поставлено в очередь до
        /// того, как можно будет поставить задачу с обычным приоритетом, если в очереди имеются
        /// задачи с высоким приоритетом.
        /// </summary>
        private const int HighPriorityTaskFactor = 3;

        /// <summary>
        /// Максимальное количество одновременно выполняемых задач. Установка значения возможна
        /// только в конструкторе.
        /// </summary>
        private readonly int mConcurrentTaskNumber;

        /// <summary>
        /// Объект синхронизации доступа к свойству <see cref="IsStopped"/>.
        /// </summary>
        private readonly object mIsStoppedLock = new object();

        /// <summary>
        /// Барьер остановки работы пула. Поднимается, когда запрошена остановка и все задачи
        /// завершили своё выполнение.
        /// </summary>
        private readonly ManualResetEvent mPoolStoppedGate = new ManualResetEvent(false);

        /// <summary>
        /// Список задач, поставленных в очередь на выполнение.
        /// </summary>
        /// <remarks>
        /// Объект синхронизации доступа <see cref="mQueuedTasksLock"/>.
        /// </remarks>
        private readonly IList<TaskListEntry> mQueuedTasks = new List<TaskListEntry>();

        /// <summary>
        /// Объект синхронизации доступа к списку задач <see cref="mQueuedTasks"/>.
        /// </summary>
        private readonly object mQueuedTasksLock = new object();

        /// <summary>
        /// Список выполняющихся в настоящий момент задач.
        /// </summary>
        /// <remarks>
        /// Объект синхронизации доступа <see cref="mRunningTasksLock"/>.
        /// </remarks>
        private readonly IList<TaskListEntry> mRunningTasks = new List<TaskListEntry>();

        /// <summary>
        /// Объект синхронизации доступа к <see cref="mRunningTasks"/>.
        /// </summary>
        private readonly object mRunningTasksLock = new object();

        /// <summary>
        /// Объект синхронизации, используемый для блокировки/запуска потока планировщика.
        /// </summary>
        private readonly object mTaskSchedulerLock = new object();

        /// <summary>
        /// Счётчик задач с высоким приоритетом, запущенных на выполнение. Каждая запущенная задача
        /// с высоким приоритетом увеличивает это значение на единицу, каждая запущенная задача с
        /// обычным приоритетом уменьшает это значение на <see cref="HighPriorityTaskFactor"/>.
        /// </summary>
        /// <remarks>
        /// Синхронизация доступа должна выполняться по средствам использования методов класса 
        /// <see cref="Interlocked"/>.
        /// </remarks>
        private int mQueuedHighPriorityTaskCounter;

        #endregion

        #region Nested type: TaskListEntry

        /// <summary>
        /// Элемент списка задач. 
        /// </summary>
        /// <remarks>
        /// Объекты после создания не изменяемы.
        /// </remarks>
        private struct TaskListEntry
        {
            #region Constructors

            /// <summary>
            /// Инициализирует новый экземпляр элемента списка задач задачей и её приоритетом.
            /// </summary>
            /// <param name="aTask">
            /// Задача.
            /// </param>
            /// <param name="aTaskPriority">
            /// Приоритет задачи.
            /// </param>
            public TaskListEntry(Task aTask, TaskPriority aTaskPriority)
            {
                mTask = aTask;
                mTaskPriority = aTaskPriority;
            }

            #endregion

            #region Public properties

            /// <summary>
            /// Задача.
            /// </summary>
            public Task Task
            {
                get { return mTask; }
            }

            /// <summary>
            /// Приоритет задачи.
            /// </summary>
            public TaskPriority TaskPriority
            {
                get { return mTaskPriority; }
            }

            #endregion

            #region Private data

            private readonly Task mTask;
            private readonly TaskPriority mTaskPriority;

            #endregion
        }

        #endregion
    }



Task
    /// <summary>
    /// Задача для выполнения в <see cref="FixedThreadPool1"/>.
    /// </summary>
    public class Task
    {
        #region Constructors

        /// <summary>
        /// Инициализирует новый экземпляр задачи для выполнения в <see cref="FixedThreadPool1"/>
        /// делегатом тела задачи.
        /// </summary>
        /// <param name="aTaskBody">
        /// Делегат тела задачи.
        /// </param>
        /// <exception cref="ArgumentNullException">
        /// Делегат тела задачи не задан.
        /// </exception>
        public Task(Action aTaskBody)
        {
            if (aTaskBody == null)
            {
                throw new ArgumentNullException("aTaskBody", "Делегат тела задачи не задан.");
            }

            TaskBody = aTaskBody;
        }

        #endregion

        #region Public properties

        /// <summary>
        /// Получает/устанавливает делегат тела задачи.
        /// </summary>
        public Action TaskBody { get; private set; }

        #endregion

        #region Events

        /// <summary>
        /// Событие, сообщающее о завершении выполнения задачи.
        /// </summary>
        public event EventHandler Finished;

        #endregion

        #region Public methods

        /// <summary>
        /// Начинает выполнение задачи.
        /// </summary>
        public void Execute()
        { 
            Thread lTaskThread =
                new Thread(
                    () =>
                        {
                            // Выполнить задачу.
                            TaskBody();
                            // Уведомить об её окончании.
                            EventHandler lFinished = Finished;
                            if (lFinished != null)
                            {
                                lFinished(this, EventArgs.Empty);
                            }
                        }) {Name = "Task thread."};
            lTaskThread.Start();
        }

        #endregion
    }



TaskPriority

    /// <summary>
    /// Приоритет задачи.
    /// </summary>
    public enum TaskPriority
    {
        /// <summary>
        /// Высокий приоритет.
        /// </summary>
        High = 0,

        /// <summary>
        /// Обычный приоритет.
        /// </summary>
        Normal,

        /// <summary>
        /// Низкий приоритет.
        /// </summary>
        Low
    }




Работа над ошибками


Спасибо большое за комментарии и конструктивную критику. Я решил, что тема будет не закрыта, если не опубликовать исправленное решение. Для начала перечислю рекомендации и опишу какие вошли в код, а какие нет, и почему.
  • Создание потока — операция очень затратная. Пул потоков нужен для того, чтобы хранить в нём несколько уже созданных потоков. Это было моей главной ошибкой и, судя по всему, именно это вызвало столь негативную реакцию со стороны технического специалиста, проверяющего моё решение. Я просто слишком сконцентрировался на условиях задачи и проглядел требование, которое следовало из самого названия класса. Спасибо, Unrul, iaroshenko. Теперь я создаю необходимое количество потоков в конструкторе пула. Класс Task больше не занимается созданием потоков.
  • Поток планировщика не нужен. Действительно, после того, как создание потока было перенесено из класса задачи в класс пула, стало возможно переложить функцию извлечения задач из очереди на сами потоки. Спасибо, iaroshenko, romik.
  • В интерфейсе Task не должно быть event. Поскольку класс задачи больше не занимается созданием потоков, событие окончания обработки задачи теперь действительно не нужен. Спасибо, Nagg.
  • Блокировку на IsStopped можно заменить обычным volatile поле. С этим я не согласен. Несмотря на утверждение MSDN о том, что благодаря модификатору volatile читающий значение такого поля код будет получать самое актуальное значение, это не так. На самом деле, если говорить упрощённо, volatile позволяет гарантировать то, что компилятор, CLR и процессор не поменяют местами чтение/запись в volatile поле и другие поля. Более подробно можно почитать здесь. Однако в моём случае блокировка IsStopped нужна для другого. Без неё потенциально возможно, что в очередь попадёт новая задача после фактической остановки пула, а также возможен случай, когда поток задачи останется в состоянии ожидания, не смотря на то, что был дан сигнал завершения. В любом случае, небольшая перестраховка в многопоточном коде никогда не повредит. Это лучше, чем потом ловить нерегулярные ошибки. Не стану с пеной у рта защищать это решение. Если кто сможет опровергнуть мой довод на основе кода, буду признателен. Спасибо, elw00d.
  • Очередь с приоритетами реализовать отдельно от логики собственно ThreadPool'а. Этого я тоже не стал делать. Фактически пул содержит не так много кода, там больше комментариев и логгирования. Но, тем не менее, основная причина не в этом. Представьте, что условие отбора изменилось. Или же нужно обеспечить возможность настройки принципа выбора следующей задачи в момент создания экземпляра пула. Придётся создавать новый класс сортированной коллекции (очереди) и, соответственно, плодить по числу классов коллекций классы пулов. Это неправильное решение. В данном случае будет лучше вынести алгоритм выбора следующей задачи в отдельный класс, реализовав шаблон Strategy. Объект «отборщика» в таком случае будет передаваться в конструктор. Но я не буду усложнять своё решение, так как по условию задачи это не требуется. Спасибо, elw00d.
  • Interlocked-методы тоже неясно зачем (если уже используются мониторы и event'ы). Эту часть я тоже не стал трогать. Interlocked-методы позволяют избежать нарушение условий отбора задач с приоритетами High и Normal. Поскольку нет блокировок на доступ к счётчику mQueuedHighPriorityTaskCounter я предпочёл на всякий случай перестраховаться, использовав Interlocked-методы. Возможно более глубокий анализ кода выявит мою неправоту, но как я уже говорил, я не самый большой специалист в многопоточности и предпочитаю подстелить соломку, тем более когда она стоит дёшево. Спасибо, elw00d.
  • Было бы очень интересно, если бы автор решил озвученные в комментариях недочеты и снова отправил в Связной обновленную версию решения. К коду это конечно имеет лишь опосредованное отношение, но тем не менее. Думаю, я не стану отправлять исправленное решение в Связной. Это будет нечестно, поскольку я воспользовался вашей помощью. Скорее всего, я отправлю им ссылку на эту статью, а там пусть сами решают нужен ли им «уже готовый специалист» или подходит человек, который умеет признавать свои ошибки и развиваться, двигаться дальше. Спасибо за поддержку, @ARC_Programmer.

Тестовый проект с исправлениями можно загрузить здесь. Исправленный код:

FixedThreadPool
// Проект: Eshva.Threading
// Имя файла: FixedThreadPool.cs
// GUID файла: 7F1EECB7-F28A-4A20-9536-26D174BCD437
// Автор: Mike Eshva (mike@eshva.ru)
// Дата создания: 04.06.2012

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Threading;


namespace Eshva.Threading.Framework
{
    /// <summary>
    /// Пул потоков, выполняющий одновременно не более определённого количество задач с учётом их
    /// приоритетов.
    /// </summary>
    /// <remarks>
    /// Тестовый проект для компании Связной.
    /// </remarks>
    public sealed class FixedThreadPool
    {
        #region Constructors

        /// <summary>
        /// Инициализирует новый экземпляр пул потоков максимальным количеством одновременно
        /// выполняемых задач.
        /// </summary>
        /// <param name="aConcurrentTaskNumber">
        /// Максимальное количество одновременно выполняемых задач.
        /// </param>
        /// <param name="aLog">
        /// Сервис логгирования.
        /// </param>
        /// <param name="aPriorityLog">
        /// Лог для вывода приоритета отобранной для выполнения задачи.
        /// </param>
        /// <exception cref="ArgumentOutOfRangeException">
        /// Неправильно заданное максимальное количество одновременно выполняемых задач.
        /// </exception>
        public FixedThreadPool
            (int aConcurrentTaskNumber, ILog aLog = null, ILog aPriorityLog = null)
        {
            if (aConcurrentTaskNumber <= 1)
            {
                throw new ArgumentOutOfRangeException(
                    "aConcurrentTaskNumber",
                    "Количество одновременно выполняемых задач должно быть больше единицы.");
            }

            Log = aLog;
            PriorityLog = aPriorityLog;

            for (int lThreadIndex = 0; lThreadIndex < aConcurrentTaskNumber; lThreadIndex++)
            {
                string lThreadName = string.Format("Task thread #{0}", lThreadIndex);
                Thread lTaskThread = new Thread(TaskThreadLogic) {Name = lThreadName};
                lTaskThread.Start();
            }
        }

        #endregion

        #region Public methods

        /// <summary>
        /// Ставит задачу <paramref name="aTask"/> в очередь на выполнение с приоритетом 
        /// <paramref name="aTaskPriority"/>.
        /// </summary>
        /// <param name="aTask">
        /// Задача для постановки в очередь на выполнения..
        /// </param>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        /// <returns>
        /// <see langword="true"/> - задача поставлена в очередь на выполнение. 
        /// <see langword="false"/> - задача не была поставлена в очередь на выполнение, так как
        /// работа пула потока была остановлена.
        /// </returns>
        /// <exception cref="ArgumentNullException">
        /// Задача для постановки в очередь на выполнения не задана.
        /// </exception>
        public bool Execute(Task aTask, TaskPriority aTaskPriority)
        {
            if (aTask == null)
            {
                throw new ArgumentNullException(
                    "aTask", "Задача для постановки в очередь на выполнения не задана.");
            }

            LogMessage("Получена новая задача для выполнения.");
            lock (mIsStoppedLock)
            {
                if (IsStopped)
                {
                    // Запрошена остановка.
                    LogPriority(aTaskPriority, ConsoleColor.DarkGray);
                    // Отклонять новые задачи.
                    return false;
                }
            }

            // Добавить задачу в очередь.
            EnqueueTask(aTask, aTaskPriority);
            LogMessage("Задача добавлена в очередь задач.", ConsoleColor.DarkYellow);

            return true;
        }

        /// <summary>
        /// Останавливает добавлении задач в очередь пула потоков, очередь не очищается. Возвращает
        /// выполнение только после окончания всех имеющихся задач в очереди.
        /// </summary>
        /// <remarks>
        /// После вызова этого метода дальнейшее добавление задач в очередь на выполнение не
        /// возможно и метод <see cref="Execute"/> будет возвращать <see langword="false"/>.
        /// Имеющиеся, на момент выполнения этого метода, задачи в очереди будут выполнены.
        /// </remarks>
        public void Stop()
        {
            // Выставить признак окончания работы пула.
            lock (mIsStoppedLock)
            {
                IsStopped = true;
                LogMessage("Запрошена остановка пула.");
            }

            // Дождаться окончания выполнения всех задач, оставшихся в очереди.
            LogMessage(
                "Начато ожидание завершения выполнения всех оставшихся задач в очереди.",
                ConsoleColor.DarkRed);
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.PulseAll(mTaskSchedulerLock);
            }

            mPoolStoppedGate.WaitOne();

            LogMessage("Дождались окончания выполнения всех задач в очереди.", ConsoleColor.DarkRed);
        }

        #endregion

        #region Private properties

        /// <summary>
        /// Получает/устанавливает признак того, остановлена ли работа пула.
        /// </summary>
        /// <value>
        /// <see langword="true"/> - работа пула остановлена, дальнейшее добавление задач в очередь
        /// не возможно. <see langword="false"/> - работа пула продолжается.
        /// </value>
        /// <remarks>
        /// Объект синхронизации доступа <see cref="mIsStoppedLock"/>.
        /// </remarks>
        private bool IsStopped { get; set; }

        /// <summary>
        /// Получает/устанавливает сервис логгирования приоритета задачи.
        /// </summary>
        private ILog PriorityLog { get; set; }

        /// <summary>
        /// Получает/устанавливает сервис логгирования сообщений.
        /// </summary>
        private ILog Log { get; set; }

        #endregion

        #region Private methods

        private void TaskThreadLogic()
        {
            lock (mTaskSchedulerLock)
            {
                while (true)
                {
                    // Отпускаем монитор и ждём сигнала, поданного через mTaskSchedulerLock.
                    Monitor.Wait(mTaskSchedulerLock);
                    lock (mQueuedTasksLock)
                    {
                        if (!mQueuedTasks.Any())
                        {
                            lock (mIsStoppedLock)
                            {
                                if (IsStopped)
                                {
                                    LogMessage(
                                        "Запрошена остановка пула и больше нет задач в очереди на выполнение.");
                                    LogMessage("Планировщик - Выход из потока планировщика.");
                                    // Сигнализировать об окончании выполнения последней задачи.
                                    mPoolStoppedGate.Set();
                                    return;
                                }
                            }
                            // Задач в очереди нет, но не было запроса на остановку пула.
                            // Ждём дальше появления задач.
                            continue;
                        }
                    }

                    LogMessage(
                        "Дождались появления задачи в очереди задач.",
                        ConsoleColor.DarkRed);
                    // Дождаться и получить следующую задачу для выполнения.
                    TaskListEntry lTask = DequeueTask();
                    LogMessage("Планировщик - Получена новая задача для выполнения.");

                    switch (lTask.TaskPriority)
                    {
                        case TaskPriority.High:
                            Interlocked.Increment(ref mQueuedHighPriorityTaskCounter);
                            break;
                        case TaskPriority.Normal:
                            Interlocked.Add(
                                ref mQueuedHighPriorityTaskCounter, -HighPriorityTaskFactor);
                            break;
                    }

                    // Запустить задачу на выполнение.
                    lTask.Task.Execute();
                    LogMessage(
                        string.Format(
                            "Планировщик - Запущена задача с приоритетом {0}.",
                            lTask.TaskPriority),
                        ConsoleColor.DarkYellow);

                    lock (mQueuedTasksLock)
                    {
                        lock (mIsStoppedLock)
                        {
                            if (IsStopped &&
                                !mQueuedTasks.Any())
                            {
                                LogMessage(
                                    "Запрошена остановка пула и больше нет задач в очереди на выполнение.");
                                LogMessage("Планировщик - Выход из потока планировщика.");
                                // Сигнализировать об окончании выполнения последней задачи.
                                mPoolStoppedGate.Set();
                                return;
                            }
                        }
                    }
                }
            }
        }

        /// <summary>
        /// Добавляет задачу <paramref name="aTask"/> в очередь задач на выполнение.
        /// </summary>
        /// <param name="aTask">
        /// Задача, добавляемая в очередь задач на выполнение.
        /// </param>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        private void EnqueueTask(Task aTask, TaskPriority aTaskPriority)
        {
            TaskListEntry lEntry = new TaskListEntry(aTask, aTaskPriority);
            LogPriority(aTaskPriority, ConsoleColor.Green);
            lock (mQueuedTasksLock)
            {
                // Добавить задачу в очередь задач на выполнение.
                mQueuedTasks.Add(lEntry);
                LogMessage(
                    string.Format(
                        "В очередь добавлена задача с приоритетом {0}", lEntry.TaskPriority),
                    ConsoleColor.Green);
                // Поднять барьер доступа к очереди задач.
                LogMessage("Поднять барьер доступа к очереди задач.", ConsoleColor.DarkRed);
            }
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }
        }

        /// <summary>
        /// Ожидает появления в очереди задач хотя бы одной задачи. Дождавшись изымает следующую
        /// задачу из очереди, учитывая правила приоритетов.
        /// </summary>
        /// <returns>
        /// Задача, изъятая из очереди задач на выполнение.
        /// </returns>
        private TaskListEntry DequeueTask()
        {
            TaskListEntry lNextTask;
            lock (mQueuedTasksLock)
            {
                lNextTask = FindNextTaskUsingPriorityRules();
                LogPriority(lNextTask.TaskPriority, ConsoleColor.Red);
                LogMessage(
                    string.Format(
                        "Получена задача из очереди задач с приоритетом {0}.",
                        lNextTask.TaskPriority),
                    ConsoleColor.DarkRed);
                mQueuedTasks.Remove(lNextTask);
            }
            lock (mTaskSchedulerLock)
            {
                // Сигнализировать об изменении в условии блокировки по mTaskSchedulerLock.
                Monitor.Pulse(mTaskSchedulerLock);
            }

            return lNextTask;
        }

        /// <summary>
        /// Находит следующую задачу с учётом правил приоритезации.
        /// </summary>
        /// <returns>
        /// Найденная следующая задача для выполнения.
        /// </returns>
        /// <remarks>
        /// ЗАМЕЧАНИЯ ПО ПРАВИЛАМ ПРИОРИТЕЗАЦИИ: Полученные мной правила выбора следующей задачи
        /// определены в задании не полностью. В частности, не было сказано что делать, если в
        /// очереди задач есть только задачи с обычным приоритетом. Также не определено, что нужно
        /// делать, если было выполнено больше, чем три задачи с высоким приоритетом. Поэтому я
        /// додумал эти правила, исходя из предположения, что лучше пусть выполняется хоть что-то,
        /// чем ждать появления задач с каким-то определённым приоритетом.
        /// </remarks>
        private TaskListEntry FindNextTaskUsingPriorityRules()
        {
            TaskListEntry lNextTask;
            lock (mQueuedTasksLock)
            {
                Debug.Assert(
                    mQueuedTasks.Count > 0,
                    "Метод FindNextTaskUsingPriorityRules не должен вызываться, если очередь задач пуста.");
                // По умолчанию будет выполняться задача с высоким приоритетом.
                TaskPriority lNextTaskPriority = TaskPriority.High;
                // Проверить возможность выполнения задачи с более низкими приоритетами.
                if (mQueuedTasks.All(aEntry => aEntry.TaskPriority == TaskPriority.Low))
                {
                    // В очереди задач все задачи с низким приоритетом.
                    // Следующая задача будет с низким приоритетом.
                    lNextTaskPriority = TaskPriority.Low;
                }
                else
                {
                    // Условие для задач с низким приоритетом не выполняется.
                    if (mQueuedTasks.Any(
                        aEntry => aEntry.TaskPriority == TaskPriority.Normal) &&
                        (mQueuedTasks.All(
                            aEntry => aEntry.TaskPriority != TaskPriority.High) ||
                         Interlocked.CompareExchange(ref mQueuedHighPriorityTaskCounter, 0, 0) >=
                         HighPriorityTaskFactor))
                    {
                        // В списке задач на выполнение есть задачи с обычным приоритетом и
                        // выполнено достаточное количество задач с высоким приоритетом или
                        // в очереди задач все задачи имеют приоритет ниже высокого.
                        // Следующая задача будет с обычным приоритетом.
                        lNextTaskPriority = TaskPriority.Normal;
                    }
                }

                lNextTask = mQueuedTasks.First(
                    aEntry => aEntry.TaskPriority == lNextTaskPriority);
            }

            return lNextTask;
        }

        /// <summary>
        /// Выводит сообщение в лог.
        /// </summary>
        /// <param name="aMessage">
        /// Сообщение.
        /// </param>
        /// <param name="aColor">
        /// Цвет сообщения.
        /// </param>
        private void LogMessage(string aMessage, ConsoleColor aColor = ConsoleColor.Yellow)
        {
            if (Log == null)
            {
                return;
            }

            Log.WriteMessage(aMessage, aColor);
        }

        /// <summary>
        /// Выводит приоритет задачи в лог.
        /// </summary>
        /// <param name="aTaskPriority">
        /// Приоритет задачи.
        /// </param>
        /// <param name="aColor">
        /// Цвет сообщения.
        /// </param>
        private void LogPriority(TaskPriority aTaskPriority, ConsoleColor aColor)
        {
            if (PriorityLog == null)
            {
                return;
            }

            string lPriority = aTaskPriority == TaskPriority.High
                                   ? "H"
                                   : aTaskPriority == TaskPriority.Normal ? "N" : "L";
            PriorityLog.WriteMessage(lPriority, aColor);
        }

        #endregion

        #region Private data

        /// <summary>
        /// Количество задач с высоким приоритетом, которое должно быть поставлено в очередь до
        /// того, как можно будет поставить задачу с обычным приоритетом, если в очереди имеются
        /// задачи с высоким приоритетом.
        /// </summary>
        private const int HighPriorityTaskFactor = 3;

        /// <summary>
        /// Объект синхронизации доступа к свойству <see cref="IsStopped"/>.
        /// </summary>
        private readonly object mIsStoppedLock = new object();

        /// <summary>
        /// Барьер остановки работы пула. Поднимается, когда запрошена остановка и все задачи
        /// завершили своё выполнение.
        /// </summary>
        private readonly ManualResetEvent mPoolStoppedGate = new ManualResetEvent(false);

        /// <summary>
        /// Список задач, поставленных в очередь на выполнение.
        /// </summary>
        /// <remarks>
        /// Объект синхронизации доступа <see cref="mQueuedTasksLock"/>.
        /// </remarks>
        private readonly IList<TaskListEntry> mQueuedTasks = new List<TaskListEntry>();

        /// <summary>
        /// Объект синхронизации доступа к списку задач <see cref="mQueuedTasks"/>.
        /// </summary>
        private readonly object mQueuedTasksLock = new object();

        /// <summary>
        /// Объект синхронизации, используемый для блокировки/запуска потока планировщика.
        /// </summary>
        private readonly object mTaskSchedulerLock = new object();

        /// <summary>
        /// Счётчик задач с высоким приоритетом, запущенных на выполнение. Каждая запущенная задача
        /// с высоким приоритетом увеличивает это значение на единицу, каждая запущенная задача с
        /// обычным приоритетом уменьшает это значение на <see cref="HighPriorityTaskFactor"/>.
        /// </summary>
        /// <remarks>
        /// Синхронизация доступа должна выполняться по средствам использования методов класса 
        /// <see cref="Interlocked"/>.
        /// </remarks>
        private int mQueuedHighPriorityTaskCounter;

        #endregion

        #region Nested type: TaskListEntry

        /// <summary>
        /// Элемент списка задач. 
        /// </summary>
        /// <remarks>
        /// Объекты после создания не изменяемы.
        /// </remarks>
        private struct TaskListEntry
        {
            #region Constructors

            /// <summary>
            /// Инициализирует новый экземпляр элемента списка задач задачей и её приоритетом.
            /// </summary>
            /// <param name="aTask">
            /// Задача.
            /// </param>
            /// <param name="aTaskPriority">
            /// Приоритет задачи.
            /// </param>
            public TaskListEntry(Task aTask, TaskPriority aTaskPriority)
            {
                mTask = aTask;
                mTaskPriority = aTaskPriority;
            }

            #endregion

            #region Public properties

            /// <summary>
            /// Задача.
            /// </summary>
            public Task Task
            {
                get { return mTask; }
            }

            /// <summary>
            /// Приоритет задачи.
            /// </summary>
            public TaskPriority TaskPriority
            {
                get { return mTaskPriority; }
            }

            #endregion

            #region Private data

            private readonly Task mTask;
            private readonly TaskPriority mTaskPriority;

            #endregion
        }

        #endregion
    }
}


Task
// Проект: Eshva.Threading
// Имя файла: Task.cs
// GUID файла: 292467E7-4816-4407-BB9B-3309D13C8614
// Автор: Mike Eshva (mike@eshva.ru)
// Дата создания: 04.06.2012

using System;


namespace Eshva.Threading.Framework
{
    /// <summary>
    /// Задача для выполнения в <see cref="FixedThreadPool"/>.
    /// </summary>
    public class Task
    {
        #region Constructors

        /// <summary>
        /// Инициализирует новый экземпляр задачи для выполнения в <see cref="FixedThreadPool"/>
        /// делегатом тела задачи.
        /// </summary>
        /// <param name="aTaskBody">
        /// Делегат тела задачи.
        /// </param>
        /// <exception cref="ArgumentNullException">
        /// Делегат тела задачи не задан.
        /// </exception>
        public Task(Action aTaskBody)
        {
            if (aTaskBody == null)
            {
                throw new ArgumentNullException("aTaskBody", "Делегат тела задачи не задан.");
            }

            TaskBody = aTaskBody;
        }

        #endregion

        #region Public methods

        /// <summary>
        /// Начинает выполнение задачи.
        /// </summary>
        public void Execute()
        {
            TaskBody();
        }

        #endregion

        #region Private properties

        /// <summary>
        /// Получает/устанавливает делегат тела задачи.
        /// </summary>
        private Action TaskBody { get; set; }

        #endregion
    }
}

Поделиться публикацией
AdBlock похитил этот баннер, но баннеры не зубы — отрастут

Подробнее
Реклама

Комментарии 51

    0
    Можно наверное было воспользоваться секцией Q&A, по сабжу — глянул только в Task и не совсем понял зачем Execute создает тред и запускает выполнение в нем? Вам в же задании написали, что интерфейс Task предоставляет только метод Execute и никаких ивентов.
      0
      Событие по идее надо было сделать internal, просто проглядел. Формально это бы не нарушило требований по интерфейсу класса. Но, думаю, это была далеко не главная проблема. Ниже люди хорошо описали бОльшую проблему моего кода — фактическое отсутствие ПУЛА потоков. Потоки повторно не использовались.
        +3
        Что касается секции Q&A, возможно, вы правы. Просто человек я на Хабре новый и ещё не до конца разобрался с её анатомией.
        +12
        Создание потока — операция очень затратная. Пул потоков нужен для того, чтобы хранить в нём несколько уже созданных потоков, обычно по числу ядер в системе. При поступлении задачи, она помещается в очередь. Когда какой-либо поток прекращает работу, он берёт очередную задачу из очереди и начинает её выполнение. В статье ParallelExtensionsExtras Tour — #7 — Additional TaskSchedulers и коде из проекта ParallelExtensionsExtras в Samples for Parallel Programming with the .NET Framework есть пример QueuedTaskScheduler, который выполняет описанные в задании требования.
          +2
          Да, вы правы. Думаю это было главной проблемой моего кода: создание потока — операция затратная. Фактически пула потоков у меня не было.
          За ссылки спасибо. Обязательно ознакомлюсь.
            0
            Если в коротко, то в Extras'ах задачу QueuedTaskScheduler решали созданием n-очередей, где n- это приоритет. И использовали для этого ConsurrentDictionary<int, ConqurrentQueue> Помоему так.

          +9
          Хотя я к Связному не имею отношения, попробую написать, что вы сделали не так.

          Задача действительно решена неверно, у вас нет никакого ThreadPool'а.
          Самая главная ошибка: каждая задача у вас выполняется в новом потоке:
          Thread lTaskThread =
          new Thread(
          () =>
          {
          // Выполнить задачу.
          TaskBody();


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

          Если бы вы были знакомы с концепцией .NET ThreadPool (и вообще паттерном thread pool), проблемы бы не было.
            0
            Да, тут грешен. Действительно, упустил из внимания, что создание потока — операция затратная. Думаю, это главная проблема.
            На самом деле есть более общий шаблон — resource pool, о нём знаю и использую. Принцип общий: есть дорогой ресурс, используй его повторно. Используется, например, с подключениями к базе данных.
            Что касается того, что все постановки задач можно делать синхронно, тут можно поспорить. Планировщик в данном случае у меня не добавляет задачи в очередь, а извлекает их из неё по мере освобождения потоков. Задачи выполняются в отдельных потоках и завершаются в произвольные моменты времени. После того, как запущенно максимальное количество потоков, планировщик должен остановиться и подождать окончания одного из них. Если отдельного потока планировщика не будет, будут останавливаться главный поток. Или я неправильно понял вашу мысль?
              0
              Сами рабочие потоки пусть из очереди и извлекают, им для этого поток-планировщик не нужен.
                0
                Потоки будут спать, пока не появится новая задача в очереди.
                Задача появилась — поток проснулся — выполнил очередную задачу — если есть еще задачи, выполнил их — опять заснул.

                Пользователь класса просто добавляет потоки в очередь и устанавливает событие, которое разбудит рабочий поток.
                  0
                  Да, возможно, вы правы. Надо ради тренировки реализовать этот вариант.
                  –8
                  Наверное скоро будет for паттерн, if паттерн и т.п. Мне одному кажеться, что за вся эта мешура мешает видеть суть?
                    0
                    Очевидно, мне одному так кажеться, ну или мы сильно в меньшинстве.
                    В качестве реабилитации, немного конструктива, то что мне кажется не правильным, помима факта неверного решения.
                    FindNextTaskUsingPriorityRules — этого метода быть не должно, зачем такие сложности и при этом нет никакой гибкости. LINQ там лишнее, простой проход в цикле был более уместен, и находил бы задачу за один проход, и был более читабельным на мой взгляд. Так еще потом при удалении задачи из списка в методе DequeueTask, потребуется еще один проход.
                    Бегло просматривая, встретил еще пару мест, где происходит примерно тоже самое.
                    Можно было организовать три очереди, чтобы исключить поиск полностью.
                    Или например то, что задача запускает поток, да возможно что автор написал так из-за того что не разобрался что такое ThreadPool, но тем не менее: задача — ее выполняют, пул потоков — тут даже нечего добавить. Логично же, что потоки должны быть в пуле?!
                    И зачем здесь патерны, вообще и в частности?! Фабрика задач, упростит понимание кода — не думаю. Или пул нужно обязательно делать синглтоном — нет, статические методы решают задачу в данном случае.
                    Я считаю патерны полезны в основном как средство как средство коммуникации между разработчиками.
                –7
                А не проще ли взять .NET4? Там за вас уже всё сделали.

                И паралельные циклы, и ограниченые пулы, и очереди с приоритетами, и массу всего что вы пытаетесь сделать.
                  +1
                  Вы, видимо, не поняли. Это тестовое задание, в котором нужно продемонстрировать своё понимание принципов работы с многопоточностью. То есть без использования базовых объектов не обойтись. А все вкусности .NET 4.0 я и сам использую с удовольствием, просто в данном контексте они бы не сработали в качестве примера, как я понял.
                  +4
                  Странная политика у связного. В принципе, это по крайней мере не быдло код, с вами могли бы побеседовать, вытянуть проблемы, о которых вам здесь написали. Уж, наверняка, сами бы догадались по фразе «Создание потока — операция очень затратная.»
                    0
                    Я общался только с кадровиком. О личных качествах (или подходах к найму сотрудников) заинтересованных лиц в проекте я ничего не знаю. Возможно, у них стоит очередь из соискателей один другого круче. Возможно, у них настолько критичный проект, что мои ошибки уронят какой-нибудь самолёт с логотипов Связной на борту. :) Кто знает?
                    +3
                    Что-то многовато блокировок. Да и вообще кода много. Кажется, что можно все это сделать проще. Например, блокировку на IsStopped можно заменить обычным volatile полем. Очередь с приоритетами реализовать отдельно от логики собственно ThreadPool'а (тут можно кстати и подумать, сделать ли её потокобезопасной или же обеспечивать потокобезопасность в вызывающем коде). А при остановке всех задач у потоков должны вызываться Thread.Join(). Interlocked-методы тоже неясно зачем (если уже используются мониторы и event'ы). В общем, имхо, вы усложнили )
                    А какие конкретно претензии по коду вам предъявили кроме «это не то, что мы телепатировали»? Да, и странно, что вам это написал кадровик, а не технический специалист.
                      0
                      По первому абзацу сейчас ничего сказать не могу, надо осмыслять. :) Но в любом случае, спасибо.
                      По поводу общения. Ничего кроме этого не предъявили, если понимать вашу фразу о телепортации, как парафраз «это далеко от истины». Да, я тоже считаю, что правила хорошего тона требуют ответа от технического специалиста, но, видимо, у них есть свои резоны поступать так. Может быть они настолько ценят своё время, что code review присланных решений у них идёт отдельной строчкой в прейскуранте. А может быть они просто используют кадровиков, как щит. Я не знаю.
                      +4
                      Связной эту задачу уже как минимум два года эту всем по сети рассылает.
                      И непонятна их цель, если такое решение их не устраивает да ещё на ту з/пл, которую они предлагают.
                        0
                        Ну, судя по комментариям, я действительно сделал как минимум одну серьёзную ошибку.
                          –3
                          Да признайтесь уже, вы не решили задачу. Точнее решили, но не ту.
                          Вы сделали не пул потоков, а класс, который позволяет отправлять задачи на решение.
                            +5
                            Дык, а это что не понятно из моих комментариев?
                          +1
                          Позвольте полубопытсвовать, во сколько Связной оценивает специалиста который готов решать им подобные задачи?
                            0
                            Цена договорная. :) Посмотрите на hh.ru текущие предложения, они пишут зарплаты (правда gross, а не net). Они эту задачу всем подряд дают уже не первый год, как я понимаю.
                        0
                        Цена договорная. :) Посмотрите на hh.ru текущие предложения, они пишут зарплаты (правда gross, а не net). Они эту задачу всем подряд дают уже не первый год, как я понимаю.
                          +3
                          Было бы очень интересно, если бы автор решил озвученные в комментариях недочеты и снова отправил в Связной обновленную версию решения. А когда все решится, то было бы интересно почитать продолжение поста (как мне видится они могут воспринять этот порыв как четкое желание работать в Связном и принять Вас на работу).
                            0
                            Желание-то есть, но Связной может не захотеть взять человека, только вчера узнавшего, что такое ThreadPool. Опыт — штука важная, и ещё таких вещей, которые надо уже знать (а не учить), много. Я, конечно, не знаю, что за специалиста они ищут и какую зп предлагают.
                              +2
                              Мы учимся постоянно — такая специализация в ИТ.
                              Если в Связном это понимают, то они должны, по идее, оценить автора по достоинству, ибо он готов, способен развиваться и двигаться дальше, достигая поставленных задач, пусть и не сразу. Думаю это важно.
                              Понятное дело — всем нужны уже «готовые» специалисты, однако не всегда можно найти такого человека. Порой смешно становится видя вакансии с жизненным циклом более года — не проще ли взять пусть не готового, но способного к саморазвитию человека?!
                            +2
                            Увидев в Task.Execute() вызов new Thread(), дальше смотреть код я бы на месте Связного не стал. Это не пул и действительно не близко к нему. Учитывая, что задача такая, которая, как вы говорите, даётся всем подряд не первый год, это фейл.
                              –3
                              Уважаемый, ты так пишешь, как-будто сам никогда не делаешь ошибок. Я же не только признаю за собой возможность ошибиться, но и не стесняюсь показать это, спросить у других совета. Сделай, пожалуйста, выводы.
                                +1
                                Делаю, конечно, why not? Если вас как-то задел способ выражение, прошу прощения. Я хотел сказать, что задание зафейлено. Имхо, ничего обидного тут нет.
                                  –8
                                  Вы затронули тему моего опыта. Опыта у меня поболее Вашего, скорее всего, будет. Заявить, что я не знаю, что такое ThreadPool, было весьма опрометчиво с Вашей стороны. Я никогда не писал свой ThreadPool, это так, но я использовал готовый множество раз. А теперь представьте, что Вам дают задание написать свой GC, пусть и за большее время. Справитесь, не забыв ни об одном свойстве оригинала? Вряд ли.
                                  Тем более Вы не написали ни одного замечания по существу, лишь воспользовались тем, что уже разглядели другие.
                                    +4
                                    Как человеку новому здесь, посоветую: никогда не переходите на личности и используйте обращение на «вы», а не на «ты». Это позволит вам сохранить карму в плюсе и полноценно участвовать в жизни сообщества.
                                      0
                                      Спасибо за совет, добрый человек, но не я начал. Пришлось открыто высказать свои претензии в ответ на открыто высказанные претензии ко мне. Посмотрите внимательно и Вы поймёте о чём я говорю.
                                      Я тоже считаю эту ветку комментариев откровенным флеймом. Antelle не сказал ничего по существу вопроса, лишь высказал своё опрометчивое мнение о моих качествах во всеуслышание. Так что тут пришлось наплевать на карму.
                                      • НЛО прилетело и опубликовало эту надпись здесь
                                          0
                                          На самом деле мне не понравилась фраза «человека, только вчера узнавшего, что такое ThreadPool». «Только вчера» узнать об этом мог лишь человек обладающий минимальным опытом. Поэтому и говорю об опыте. Да, я упустил из внимания это требования, наличие которого следует из названия класса, сконцентрировавшись на явных требованиях, указанных в описании задачи. Эту свою ошибку я признал и неоднократно, посмотрите в моих комментариях. Зачем было делать намёк на мой опыт, я не понимаю, поэтому и вскипятился. Надеюсь, я достаточно ясно выразил свою позицию, и мы больше не будем разводить флейм по этому вопросу.
                                          Извинения Antelle принимаю, лишь хотел объяснить, что конкретно меня задело, чтобы человек не делал эту ошибку в будущем по отношению к кому бы то ни было.
                                            +2
                                            да ладно, не напрягайтесь, здесь часто говорят очевидные вещи и выпадают из контекста.
                                            но и полезного много можно услышать.
                                            рекомендую игнорировать то что не нравится и отвечать только там где есть что-то полезное.
                                              –1
                                              Уже не напрягаюсь, спасибо. Когда будет возможность плюсовать/минусовать буду просто минусовать в таких случаях.
                                              Работаю над ошибками. Сейчас обновлю статью, если это можно сделать на Хабре, добавлю исправленную реализацию.
                                                0
                                                Вот, честное слово, не пойму, за что минусуете предыдущее моё высказывание. Я же не сказал, что буду «срать в тапки» «гадить в карму». Неужели это так крамольно минусовать неконструктивные комментарии?
                                                  +2
                                                  >> Я же не сказал, что буду «срать в тапки» «гадить в карму»
                                                  >> Когда будет возможность плюсовать/минусовать буду просто минусовать в таких случаях.

                                                  Иногда бывает лучше просто успокоиться и закончить переписку.
                                                  0
                                                  Да что вы дерзкий-то такой.
                                                  Вы пришли за советом, вам его дали, попутно покритиковали — но вы же сами попросили, не мы же к вам пришли в дом и начали код обсуждать.

                                                  А вы тут сразу на «ты» перешли, минусами угрожаете, считаете себя в области GC. Ну расскажите мне про GC в Go например, там он довольно необычен, но Вы-то специалист, ничего не упускаете из виду.
                                                    0
                                                    За конструктивную критику я уже высказал благодарности, за неконструктивную поругался. Думаю, Вы бы сделали тоже самое на моём месте. Читайте обновлённый вариант статьи.
                                                    Давайте уже, действительно, заканчивать флейм, в этом Вы правы. Я продолжать точно не буду, так что можете сказать своё последнее веское слово, и закончим. :)
                                0
                                забей,
                                т.к. к программированию данный вопрос не имеет отношения, ибо ты получил ответ HR, а не от тим-лидера, или архитектора и даже не от разработчика…
                                это даже не собеседование — ты не получил фидбека от человека, который давал оценку твоему решению…

                                p.s. вспоминаю собеседования в сибоссе в начале 2000х которые были апофеозом страстей и тщеславия топ-менеджмента… и теперь привели контору в глубокую ж…
                                  0
                                  Да я и не парюсь на тему работы в Связном, просто хотелось дойти до истины, что у меня не так в коде, а для этого лучше всего подходит коллективный разум. Заодно получилась неплохая статья. Жаль, что кто-то анонимно накакал в карму, так что пока что писать не смогу. Видимо, это тут в порядке вещей.
                                  За напоминание о Сибоссе отдельное спасибо. Как раз недавно вспоминал название этой фирмы. Тоже рассматривал их предложение году этак в 2003. Но мне, для принятия решения даже не соваться в эту кантору, хватило простого взгляда на здание, где располагался их офис, и пары разговоров с работниками, вышедшими покурить.
                                  0
                                  Вот эти конструкции ни к чему, как мне кажется. Разве только volatile на IsStopped.
                                  lock (mIsStoppedLock)
                                  {
                                  if (IsStopped)

                                  цитата из сэмплов, что в комплекте с 10ой студией.
                                  while (!_shouldStop)
                                  {

                                  }
                                    0
                                    Не обновилась страница, уже оказывается обсуждалось.
                                    ЗЫ мне кажется можно решить без блокировки.
                                    0
                                    Вот я смотрю на итоговый результат и что сразу же вижу, что вызов Task.Execute(); находится внутри всех блокировок, решение банально не работает. Сейчас илллюзия работы, так как время задач слишком маленькое, меняем Thread.Sleep(lRandom.Next(50, 500)); на Thread.Sleep(10000)); и видим трогательное исполнение 10-секундных задач фактически в одном потоке.
                                    Первоначальное решение не видел, но сейчас это действительно «совсем не то». Но это не простая тема, такие ошибки даже в серьезных книгах есть, вот, например, мой вопрос на Stackoverflow про нерабочий пример в Accelerated C# 2008.
                                      0
                                      Первоначальный вариант был ВООБЩЕ не то, в отличие от нынешнего «совсем не то».

                                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                    Самое читаемое