Thread pool #3

MikePuzanov · 2021-10-20T18:31:52Z

No description provided.

yurii-litvinov · 2021-10-24T12:52:39Z

ThreadPool/ThreadPool/IMyTask.cs

+namespace ThreadPool
+{
+    /// <summary>
+    /// task interface


Понятно что это интерфейс, но что это за интерфейс?

yurii-litvinov · 2021-10-24T12:54:42Z

ThreadPool/ThreadPool/MyTask.cs

+namespace ThreadPool
+{
+    /// <summary>
+    /// task classs


Тоже не очень, даже более не очень, чем в предыдущем случае :)

yurii-litvinov · 2021-10-24T12:56:22Z

ThreadPool/ThreadPool/MyTask.cs

+    /// <summary>
+    /// task classs
+    /// </summary>
+    public class MyTask<TResult> : IMyTask<TResult>


Его лучше сделать private- вложенным классом для MyThreadPool, чтобы у пользователей не было соблазна создавать его самому, вызывать Count и делать прочие ненужные вещи.

yurii-litvinov · 2021-10-24T12:57:40Z

ThreadPool/ThreadPool/MyTask.cs

+        /// </summary>
+        public MyTask(Func<TResult> function)
+        {
+            _func = function ?? throw new ArgumentException();


Можно более специфично, ArgumentNullException(nameof(function));

yurii-litvinov · 2021-10-24T12:58:51Z

ThreadPool/ThreadPool/MyTask.cs

+                _resultException = funcException;
+            }
+
+            lock (_locker)


Это, как я понимаю, задел на будущее, потому что пока синхронизироваться не с кем

yurii-litvinov · 2021-10-24T12:59:40Z

ThreadPool/ThreadPool/MyThreadPool.cs

+        {
+            if (countThreads < 1)
+            {
+                throw new ArgumentException();


ArgumentOutOfRangeException(nameof(countThreads));

yurii-litvinov · 2021-10-24T13:02:53Z

ThreadPool/ThreadPool/MyThreadPool.cs

+            }
+
+            var task = new MyTask<TResult>(function);
+            _tasksQueue.Enqueue(task.Count);


Не-а, если в этот момент сделают Shutdown и остановят все треды, задача добавится в очередь, но делать её будет некому. Получится задача-зомби --- она как бы жива, но не работает, и любой, кто вызовет её Result, задедлочится

ThreadPool/ThreadPool/MyThreadPool.cs

+            lock (_lockObject)
+            {
+                _token.Cancel();
+            }


yurii-litvinov · 2021-10-24T13:04:03Z

ThreadPool/ThreadPool/Program.cs

+        static void Main(string[] args)
+        {
+            Console.WriteLine("Hello World!");
+        }


Это не нужно, собирайте библиотеку просто

yurii-litvinov · 2021-10-24T13:06:25Z

ThreadPool/ThreadPoolTest/ThreadPoolTest.cs

+                _threadPool.AddTask(() =>
+                {
+                    Interlocked.Increment(ref number);
+                    Thread.Sleep(3000);


Для этого есть всякие ManualResetEvent-ы, а то очень долго тесты исполняться будут

yurii-litvinov · 2021-12-01T16:31:17Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                }
+            }
+
+            var task = new MyTask<TResult>(function, this);


Мм, и в этот момент происходит Shutdown, все потоки останавливаются, но мы уже не можем отменить добавление таска.

yurii-litvinov · 2021-12-01T16:46:06Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                    while (_continueWithTasksQueue.Count > 0)
+                    {
+                        var action = _continueWithTasksQueue.Dequeue();
+                        lock (_locker)


Двойной lock на _locker? Если бы lock в C# был не рекурсивным, был бы дедлок :) Может, имелся в виду lockObject из тредпула? Но тогда доверили бы тредпулу им управлять

yurii-litvinov · 2021-12-01T16:46:38Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                        lock (_locker)
+                        {
+                            _myThreadPool._tasksQueue.Enqueue(action);
+                            _waiterManual.Set();


Он и так выставлен тут уже. Или, опять-таки, имелся в виду флаг из тредпула?

yurii-litvinov · 2021-12-01T16:48:45Z

ThreadPool/ThreadPool/MyThreadPool.cs

+
+                    var task = new MyTask<TNewResult>(() => func(Result), _myThreadPool);
+                    _continueWithTasksQueue.Enqueue(task.Count);
+                    if (IsCompleted)


Если IsCompleted, можно просто добавлять таск на тредпул, даже лочить никого не надо. О задачах из очереди позаботится Count. А то тут как-то очень много всего в критической секции.

yurii-litvinov

Надо ещё немного поправить

ThreadPool/ThreadPool/MyThreadPool.cs

+
+namespace ThreadPool
+{
+    public class MyThreadPool


ThreadPool/ThreadPool/MyThreadPool.cs

+            }
+            lock (_lockObject)
+            {
+                var task = new MyTask<TResult>(function, this);


yurii-litvinov · 2021-12-20T15:19:53Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                        lock (_myThreadPool._lockObject)
+                        {
+                            _myThreadPool._tasksQueue.Enqueue(action);
+                            _myThreadPool._waiterNewTask.Set();
+                        }


Это лучше доверить самому тредпулу. И это не синхронизировано с Shutdown, так что хоть, кажется, и будет работать (потому что у Вас потоки останавливаются только когда очередь пуста, а тут она гарантированно не пуста, потому что какой-то поток нас сейчас как раз и считает на 39-й строчке, и мы ему ещё задач накидаем), но это ни разу не очевидно и стоило хотя бы в комментарии написать.

yurii-litvinov · 2021-12-20T15:20:53Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                    {
+                        throw new InvalidOperationException();
+                    }
+                }


Лучше сделайт требпулу приватный метод "Готов принимать новые задачи" и вызывайте его тут и в AddTask. А то это какое-то вмешательство в личную жизнь

ThreadPool/ThreadPool/MyThreadPool.cs

+                    }
+                }
+
+                var task = new MyTask<TNewResult>(() => func(Result), _myThreadPool);


yurii-litvinov · 2021-12-20T15:23:08Z

ThreadPool/ThreadPoolTest/ThreadPoolTest.cs

+            Assert.AreEqual(16, task2.Result);
+            Assert.AreEqual(28, task3.Result);
+        }
+    }


Надо бы ещё тесты на параллельные запросы к тредпулу (например, Shutdown и AddTask, что ничего не дедлочится)

Всё ещё нужно. Кажется у Вас методы самого тредпула из разных потоков нигде не вызываются.

yurii-litvinov · 2022-03-07T11:15:19Z

ThreadPool/ThreadPool/IMyTask.cs

+using System;
+
+namespace ThreadPool
+{


Suggested change

using System;

namespace ThreadPool

{

namespace ThreadPool;

using System;

И дальше без лишнего отступа, здесь и ниже. C# 10 же.

yurii-litvinov · 2022-03-07T11:23:01Z

ThreadPool/ThreadPool/MyThreadPool.cs

+                if (_token.IsCancellationRequested)
+                {
+                    throw new InvalidOperationException();
+                }


Тут тоже можно было вызвать ReadyToAddNewTask()

yurii-litvinov · 2022-03-07T11:24:48Z

ThreadPool/ThreadPool/MyThreadPool.cs

+            {
+                _waiterNewTask.Set();
+                _waiterTaskDone.WaitOne();
+                _waiterTaskDone.Reset();


WaitOne же и так его опустит, это AutoResetEvent. По идее, этот Reset ничего не делает.

yurii-litvinov · 2022-03-07T11:26:24Z

ThreadPool/ThreadPool/MyThreadPool.cs

+        private readonly ConcurrentQueue<Action> _tasksQueue = new();
+        private readonly AutoResetEvent _waiterNewTask = new AutoResetEvent(false);
+        private readonly AutoResetEvent _waiterTaskDone = new AutoResetEvent(false);
+        private int _freeThreadsCount = 0;


Это должно быть volatile, потому что используется в Shutdown и рабочих потоках несинхронизированно. Есть риск дедлока в Shutdown, когда мы прочитали старое значение и пошли внутрь цикла, когда все потоки уже остановлены. Или в Shutdown Volatile.Read делать.

yurii-litvinov · 2022-03-07T11:31:15Z

ThreadPool/ThreadPool/MyThreadPool.cs

+            /// </summary>
+            public MyTask(Func<TResult> function, MyThreadPool myThreadPool)
+            {
+                _func = function ?? throw new ArgumentNullException(nameof(function));


Есть удобный метод ArgumentNullException.ThrowIfNull

yurii-litvinov · 2022-03-07T11:36:28Z

ThreadPool/ThreadPoolTest/ThreadPoolTest.cs

+            Assert.AreEqual(16, task2.Result);
+            Assert.AreEqual(28, task3.Result);
+        }
+    }


Всё ещё нужно. Кажется у Вас методы самого тредпула из разных потоков нигде не вызываются.

yurii-litvinov

Комментарии к коммитам ужасны. В остальном всё ок, зачтена.

lykwer added 2 commits October 10, 2021 16:57

base files

81b20c7

add test

989fa25

yurii-litvinov requested changes Oct 24, 2021

View reviewed changes

lykwer added 4 commits November 22, 2021 19:44

fix code

80dba3b

fix

654b4aa

fix test

c1122bf

delete commenting string

f580fa2

yurii-litvinov requested changes Dec 1, 2021

View reviewed changes

lykwer added 2 commits December 7, 2021 00:08

fix code

7c3d283

fix deadlock

f6e2090

yurii-litvinov requested changes Dec 20, 2021

View reviewed changes

lykwer added 3 commits December 22, 2021 17:04

fix

b48bb23

change

8e50071

update appveyor.yml

0dce30f

yurii-litvinov requested changes Mar 7, 2022

View reviewed changes

lykwer added 4 commits March 9, 2022 13:45

fix

a692a03

fix code and add test

07da886

fix test

13d7c71

fix

638fce6

yurii-litvinov approved these changes Apr 1, 2022

View reviewed changes

Thread pool #3

Are you sure you want to change the base?

Thread pool #3

Uh oh!

Conversation

MikePuzanov commented Oct 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurii-litvinov left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurii-litvinov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants