My ThreadPool by Niksen111 · Pull Request #5 · Niksen111/spbuHomeworksSem3

Niksen111 · 2022-11-30T17:15:08Z

Simple thread pool

3Homework12.10.22/MyThreadPool/MyThreadPool.Tests/UnitTest1.cs

yurii-litvinov · 2022-12-26T19:48:12Z

3Homework12.10.22/MyThreadPool/MyThreadPool.Tests/UnitTest1.cs

+
+using NUnit.Framework;
+
+public class Tests


А ещё линтер очень недоволен, и тут даже по делу

yurii-litvinov · 2022-12-26T19:48:18Z

3Homework12.10.22/MyThreadPool/MyThreadPool.Tests/UnitTest1.cs

+    [Test]
+    public void Test1()
+    {
+        Assert.Pass();


yurii-litvinov · 2022-12-26T19:49:16Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyTask.cs

@@ -0,0 +1,62 @@
+namespace MyThreadPool;
+
+public class MyTask<T> : IMyTask<T>


Надо комментарии

Это лучше сделать private-вложенным классом ThreadPool-а, чтобы извне нельзя было делать странные вещи, например, вызывать Start

yurii-litvinov · 2022-12-26T19:49:40Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyTask.cs

+    /// <inheritdoc/>
+    public bool IsCompleted { get; private set; } = false;
+
+    public MyTask(Func<T> func, MyThreadPool threadPool)


Тут nullability-анализ совсем недоволен

yurii-litvinov · 2022-12-26T19:50:05Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyTask.cs

+    private Func<T> func;
+    private ManualResetEvent reset = new(false);
+    private T result;
+    private Exception? retrunedException;


retruned :)

yurii-litvinov · 2022-12-26T19:50:59Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyTask.cs

+    {
+        if (!this.IsCompleted)
+        {
+            this.reset.WaitOne();


Это прямо нарушает требование условия про неблокирование вызывающего

yurii-litvinov · 2022-12-26T19:51:54Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+        while (this.tasks.Count > 0)
+        {
+        }


Крутящееся ожидание нельзя, процессор сожжёте :)

yurii-litvinov · 2022-12-26T19:52:40Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyTask.cs

+                throw new AggregateException(this.retrunedException);
+            }
+
+            return this.result;


Что-то я не понял, как в result в итоге оказывается значение

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+        {
+            while (!token.IsCancellationRequested)
+            {
+                if (this.collection.TryTake(out var action))


yurii-litvinov

Существенно лучше, чем было, но существенно хуже, чем могло бы быть

yurii-litvinov · 2023-02-11T17:07:21Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+        for (int i = 0; i < 20 && this.tasks.Count > 0; ++i)
+        {
+            Thread.Sleep(1000);
+        }


Так оно заблокирует вызывающего на целую секунду минимум, так нельзя. Рабочие потоки могут сами сообщать, что они всё, например, выставляя какой-нибудь AutoResetEvent, который мы тут можем ждать. И параллельно с этим запустить поток, висящий на ThreadSleep 20 секунд, и тоже выставляющий Event — типа таймаут. В реальной жизни это был бы Task.WhenAny, но в этой задаче Task.WhenAny нельзя — слишком высокоуровневый.

yurii-litvinov · 2023-02-11T17:28:52Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+                }
+                else
+                {
+                    Thread.Sleep(1000);


Секунда для процессора — это целая вечность. Типа "пытаемся взять задачу, а если она не нашлась, засыпаем на эпохи, пока звёзды не займут правильное положение". И нет, тут Sleep не надо, мы можем при добавлении задачи в очередь будить поток — используйте ResetEvent-ы.

yurii-litvinov · 2023-02-11T17:30:10Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+    {
+        private MyThreadPool pool;
+        private Func<T> func;
+        private ManualResetEvent reset = new(false);


Это прежде всего не reset, а event, так что и называться должен так, чтобы отражать событие, которое представляет.

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+            {
+                this.reset.Set();
+                this.IsCompleted = true;
+            }


yurii-litvinov · 2023-02-11T17:44:36Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+                this.reset.Set();
+                this.IsCompleted = true;


Наоборот. Иначе будет задача, которая уже вернула результат, но ещё не IsCompleted, это не обязательно баг, но нездорово

yurii-litvinov · 2023-02-11T17:46:15Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+        /// <inheritdoc/>
+        public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<T, TNewResult> func1)
+        {
+            return this.pool.Submit(() => func1(this.Result));


Не-а. Если есть длинная задача, у которой 150 Continuation-ов, то они все попадут в пул и будут ждать, пока задача закончится (встав на ResetEvent в Result). Остальные задачи в пуле не смогут считаться, потому что потоки пула заняты. Надо, чтобы Continuation-ы попадали в пул только когда они готовы считаться.

3Homework12.10.22/MyThreadPool/MyThreadPoolTests/MyThreadPoolTests.cs

+
+        pool.Shutdown();
+    }
+}


…oved to a separate thread.

yurii-litvinov

Ещё пару важных моментов всё-таки надо поправить

yurii-litvinov · 2023-02-13T10:05:55Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+        });
+
+        timeoutThread.Start();
+        this.tasksOver.WaitOne();


tasksOver выставляется каждым рабочим потоком, когда он видит пустую очередь (например, в самом начале) и нигде не сбрасывается вроде. Если это так, то тут мы сразу же проскочим этот WaitOne, вне зависимости от занятости потоков.

yurii-litvinov · 2023-02-13T10:10:12Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+    {
+        var task = new MyTask<TResult>(func, this);
+        this.tasks.Add(() => task.Start());
+        this.taskAdded.Set();


taskAdded — это AutoResetEvent, так что если мы вызвали десять раз Submit до активации какого-то из рабочих потоков, у нас будет десять задач в очереди и ровно один поток, который разбужен, чтобы их делать. Не очень хорошо.

yurii-litvinov · 2023-02-13T13:26:35Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+            }
+
+            var newTask = new MyTask<TNewResult>(NewFunc, this.pool);
+            var thread = new Thread(() => newTask.Start());


А так continuation запускается вообще в отдельном потоке, что, конечно, разгружает тредпул, но убивает всю идею тредпула :) Нет, так нельзя, надо класть на пул, но потом, по завершению основной задачи.

yurii-litvinov · 2023-02-15T11:53:27Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

    private bool isShutdown;
    private AutoResetEvent tasksOver;
-    private AutoResetEvent taskAdded;
+    private Semaphore taskCount = new Semaphore(0, 10000);


Почему 10000? Больше 10000 пул не принимает? Нет, пул должен принимать сколько угодно задач. Можно без семафора, на Event-ах :)

yurii-litvinov · 2023-02-15T11:57:00Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

        for (int i = 0; i < this.ThreadCount; i++)
        {
-            this.taskAdded.Set();
+            this.taskCount.Release();


Release имеет аргумент "сколько зарелизить", так что можно было и не в цикле

yurii-litvinov · 2023-02-15T12:00:46Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

        var task = new MyTask<TResult>(func, this);
        this.tasks.Add(() => task.Start());
-        this.taskAdded.Set();
+        this.tasksOver.Reset();


Это не синхронизировано с Set в рабочем потоке, поэтому, кажется, может произойти "Рабочий поток увидел, что в очереди пусто, приготовился сделать tasksOver.Set, но тут произошло переключение контекстов, управление передалось в Submit, он сделал this.tasksOver.Reset();, управление вернулось в рабочий поток, он тут же сделал Set и задача в очереди, но задачи типа кончились."

yurii-litvinov · 2023-02-15T12:04:36Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+                return this.pool.Submit(() => func1(this.Result));
+            }
+
+            MyTask<TNewResult> newTask = new MyTask<TNewResult>(() => func1(this.Result), this.pool);


Два раза один тип в одной строке не пишут :)

yurii-litvinov · 2023-02-15T12:07:08Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+            {
+                this.deferredTasks.Add(() => newTask.Start());
+            }
+            catch (InvalidOperationException)


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

yurii-litvinov · 2023-02-15T12:09:14Z

3Homework12.10.22/MyThreadPool/MyThreadPool/MyThreadPool.cs

+                this.deferredTasks.CompleteAdding();
+                foreach (var task in this.deferredTasks)
+                {
+                    this.pool.Submit(() => task);


А если к этому моменту пулу сделали Shutdown, Submit бросит исключение --- в рабочий поток, так что пользователь ничего про него не узнает. А рабочий поток бесславно сдохнет. Нехорошо.

Niksen111 added 7 commits September 17, 2022 15:09

Added gitingore

3993c24

Created project, created MyThreadPool class

2150359

Added NUnit project

1838e3e

Added myThreadClass

826a1f6

Added comments

6ae9ecf

MyThreadPool is done

cb67db8

MyTask is done

b5bef33

yurii-litvinov suggested changes Dec 26, 2022

View reviewed changes

Niksen111 added 6 commits December 27, 2022 13:03

Added comment

76aca30

Fixed thread pool, added tests

c7f3fe8

Fixed smth

6ec01e5

MyTask class moved to MyThreadPool class

c98b6be

Writed tests

5b4d94a

Fixed style

e35b92f

yurii-litvinov suggested changes Feb 11, 2023

View reviewed changes

Niksen111 added 4 commits February 12, 2023 13:43

Removed unnecessary 'Sleep'

6e4d569

Waiting for the result of the previous action about ContinueWith is m…

dd73d84

…oved to a separate thread.

Added tests

513bea3

Fixed style

6deb243

yurii-litvinov suggested changes Feb 13, 2023

View reviewed changes

Niksen111 added 2 commits February 14, 2023 14:47

Fixed threads waiting

f6b6880

Fixed continueWith

fcbb680

yurii-litvinov suggested changes Feb 15, 2023

View reviewed changes

		@@ -0,0 +1,62 @@
		namespace MyThreadPool;

		public class MyTask<T> : IMyTask<T>

Comments

Conversation

Niksen111 commented Nov 30, 2022

Uh oh!

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!

This comment was marked as resolved.

Uh oh!

yurii-litvinov left a comment

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!

This comment was marked as resolved.

Uh oh!

yurii-litvinov left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants