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

+    /// <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

+    /// <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

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


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

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

+    /// 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

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


Это как?..

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

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

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


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

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

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


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

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

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


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

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

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


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

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

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


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

YuriUfimtsev

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

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

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


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

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

+    /// 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

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


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

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

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


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

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

+        /// <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

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


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

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

+        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

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


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

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

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


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

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

+
+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

YuriUfimtsev Nov 4, 2023 •

edited

Loading

YuriUfimtsev Dec 21, 2023 •

edited

Loading