My thread pool by Palezehvat · Pull Request #22 · Palezehvat/HomeworksCSharp

Palezehvat · 2023-10-06T17:14:44Z

No description provided.

YuriUfimtsev · 2023-11-03T19:47:36Z

MyThreadPool/MyThreadPool/IMyTask.cs

+    /// <summary>
+    /// A method for solving subtasks from the results obtained from the tasks
+    /// </summary>
+    public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> suppiler);


Комментарии должны присутствовать у всех public-методов, а не выборочно :)

YuriUfimtsev · 2023-11-03T19:48:23Z

MyThreadPool/MyThreadPool/IMyTask.cs

+    /// <summary>
+    /// A method for solving subtasks from the results obtained from the tasks
+    /// </summary>
+    public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> suppiler);


supplier
Здесь и в других местах

YuriUfimtsev · 2023-11-04T10:14:54Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+    private readonly CancellationTokenSource token = new();
+    private readonly Object lockerForThreads;
+    private readonly Object lockerForTasks;
+    private volatile bool stopCount = false;


Компилятор ругается, что значение поля нигде не используется

YuriUfimtsev · 2023-11-04T10:21:21Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+    /// Accepts a function, adds it as a task in the thread, and returns the created task
+    /// </summary>
+    public IMyTask<TResult> Submit<TResult>(Func<TResult> suppiler)
+    {


Здесь надо учесть условие "после вызова Shutdown новые задачи не принимаются на исполнение потоками из пула", а также согласовать исполнение метод с Shutdown (сделать так, чтобы не было гонок при вызове Submit и Shutdown одновременно из разных потоков)

Согласование Submit с Shutdown -- не готово.
(для этого надо использовать lock по защищаемому ресурсу)

YuriUfimtsev · 2023-11-04T10:23:09Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+        if (token.IsCancellationRequested)
+        {
+            ;
+        }


Это как?..

Видимо, поле stopCount создавалось как показатель, был ли уже запрошен Shutdown. Тогда надо его либо свойством сделать, либо вообще убрать. Если свойством сделать, то у stopCount будет хоть какое-то гипотетическое применение в виде запрашивания извне.
И назвать тогда как-то понятнее, типо IsShutdownRequested

YuriUfimtsev · 2023-11-04T11:35:08Z

MyThreadPool/MyThreadPool/MyTask.cs

+            isCompleted = true;
+            while (queueWithContinueWithTasks.Count > 0)
+            {
+                queueWithTasks.Enqueue(queueWithContinueWithTasks.Dequeue());


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

YuriUfimtsev · 2023-11-04T11:39:38Z

MyThreadPool/MyThreadPool/MyTask.cs

+    private volatile bool isCompleted = false;
+    private TResult? result;
+    private readonly Queue<Action> queueWithContinueWithTasks;
+    private readonly MyThread[] arrayThreads;


Зачем это поле?

YuriUfimtsev · 2023-11-04T11:41:42Z

MyThreadPool/MyThreadPool/MyTask.cs

+    }
+
+    public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> suppiler)
+    {


Здесь тоже прямую работу с очередью задач пула надо заменить на Submit. Иначе если кто-угодно будет его очередь менять, сложно будет найти ошибку. Да и не в духе ООП это

YuriUfimtsev · 2023-11-04T11:47:02Z

MyThreadPool/MyThreadPool/MyTask.cs

+    }
+
+    public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> suppiler)
+    {


Здесь тоже надо позаботиться об отсутствии гонок между ContinueWith и Shutdown, между добавлением continuation-ов на пул и Shutdown (используя lock(и))

YuriUfimtsev · 2023-11-04T11:54:46Z

MyThreadPool/TestsForMyThreadPool/TestsForMyThreadPool.cs

+        Assert.That(Equals(myTask.Result, "16"));
+        myThreadPool.Shutdown();
+    }
+}


Тестов стоит добавить: тест, проверяющий, что в пуле действительно не менее n потоков; тест на корректность IsCompleted, Result (в случае с исключением); тест на Shutdown (проверить, что "уже запущенные задачи не прерываются, но новые задачи не принимаются на исполнение потоками из пула"); тесты на конкурентный доступ к методам пула из нескольких потоков (submit задач из разных потоков, Submit и Shutdown одновременно)

YuriUfimtsev

Уже лучше, но исправлено не всё. И куда-то исчез механизм работы с continuation-ами задач

YuriUfimtsev · 2023-12-21T14:05:56Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+                    if (token.IsCancellationRequested)
+                    {
+                        return;
+                    }


Это зачем? Само выйдет

YuriUfimtsev · 2023-12-21T15:23:16Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+    /// Accepts a function, adds it as a task in the thread, and returns the created task
+    /// </summary>
+    public IMyTask<TResult> Submit<TResult>(Func<TResult> suppiler)
+    {


Согласование Submit с Shutdown -- не готово.
(для этого надо использовать lock по защищаемому ресурсу)

YuriUfimtsev · 2023-12-21T15:27:36Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+
+            if (!token.IsCancellationRequested)
+            {
+                return pool.Submit(() => supplier(Result));


Ниже компилятор ругается: действительно, почему supplier может быть null?

YuriUfimtsev · 2023-12-21T15:28:26Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+                waitHandle.WaitOne();
+                if (!token.IsCancellationRequested)
+                {
+                    lock (locker)


Здесь lock по очереди задач надо, как ранее писал

YuriUfimtsev · 2023-12-21T15:34:50Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+        /// <summary>
+        /// A method for solving subtasks from the results obtained from the tasks
+        /// </summary>
+        public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> supplier)


Здесь и в StartSupplier куда-то пропала функциональность по добавлению всех continuation-ов в очередь и сабмиту этих continuation-ов в тредпул, когда "базовая" задача досчиталась. А это для выполнения условий задачи важно

YuriUfimtsev · 2023-12-21T15:40:19Z

MyThreadPool/TestsForMyThreadPool/TestsForMyThreadPool.cs

+        Assert.That(firstTask.Result, Is.EqualTo(4));
+        Assert.That(secondTask.Result, Is.EqualTo(6));


А вот это хорошо, никаких Assert.That(Equals(..)) 👍

YuriUfimtsev · 2023-12-21T15:43:12Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+        private TResult? result;
+        private Exception? exception;
+        private CancellationTokenSource token;
+        private EventWaitHandle waitHandle = new EventWaitHandle(false, EventResetMode.ManualReset);


Эквивалентно

Suggested change

private EventWaitHandle waitHandle = new EventWaitHandle(false, EventResetMode.ManualReset);

private ManualResetEvent resetEvent = new ManualResetEvent(false);

Удобнее использовать и более распространено :)
Для пропуска одного потока, а не всех сразу, есть AutoResetEvent

YuriUfimtsev · 2023-12-21T15:46:20Z

MyThreadPool/MyThreadPool/MyThreadPool.cs

+    /// </summary>
+    public MyThreadPool(int sizeThreads)
+    {
+        waitHandle = new EventWaitHandle(false, EventResetMode.ManualReset);


Здесь тоже лучше ManualResetEvent использовать. Хотя если вы его используйте для пропуска одного потока для выполнения задачи при Submit, то лучше AutoResetEvent. MRE только все потоки умеет пропускать за раз

YuriUfimtsev · 2023-12-21T15:51:50Z

MyThreadPool/TestsForMyThreadPool/TestsForMyThreadPool.cs

+
+    [Test]
+    public void NumberThreadsTheRequiredNumberIsCreated()
+    {


Не, так просто не получится. Вдруг GetNumberOfThreads просто заданное число возвращает :)
Здесь надо загрузить все потоки пула задачами и проверить, что новая добавленная задача не будет принята на исполнение. Для того, чтобы загрузить потоки задачами, внутри задач можно использовать ManualResetEvent

YuriUfimtsev · 2023-12-21T15:53:00Z

MyThreadPool/TestsForMyThreadPool/TestsForMyThreadPool.cs

+
+using MyThreadPool;
+
+public class Tests


Тестов на конкурентный доступ к методам пула по-прежнему не хватает

palorel added 11 commits September 29, 2023 21:43

Начало работы

819d66b

Добавление IMyTask и изменение кода

76f2240

Улучшение

4449498

Тесты и изменения

0351153

Ревью тестов

340e4db

Изменение .csproj

5ae1492

Проверка первого теста

1cdf7c7

Все тесты

b5059ad

Попытка тестирования

2fb9b31

Качественное улучшение

b442b2a

Тест на разных устройствах способен давать разный результат, не подходит

82702d5

YuriUfimtsev reviewed Nov 4, 2023

View reviewed changes

palorel added 5 commits December 10, 2023 12:27

Ревью

5bd3f9a

Проверка

c9b8fc6

Попытка проверки 2

f796dda

Попытка проверки 3

a158794

Проверка 4

2390068

YuriUfimtsev reviewed Dec 21, 2023

View reviewed changes

Часть ревью

5c81b30

		Assert.That(firstTask.Result, Is.EqualTo(4));
		Assert.That(secondTask.Result, Is.EqualTo(6));

	private EventWaitHandle waitHandle = new EventWaitHandle(false, EventResetMode.ManualReset);
	private ManualResetEvent resetEvent = new ManualResetEvent(false);

Conversation

Palezehvat commented Oct 6, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriUfimtsev Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriUfimtsev 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!

YuriUfimtsev Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

3 participants

Comments

YuriUfimtsev Nov 4, 2023 •

edited

Loading

YuriUfimtsev Dec 21, 2023 •

edited

Loading