Conversation
C#/forSpbu/MyThreadPool/IMyTask.cs
Outdated
There was a problem hiding this comment.
Если метод возвращает значение, то это не просто так. Здесь стоит его получить и в if учесть
There was a problem hiding this comment.
Надо в цикл добавить использование ResetEvent-а\ов, чтобы не было крутящейся блокировки, и потоки при отсутствии задач на WaitOne стояли. Потому что в случае с WaitOne происходит системный вызов, и пока Set не установят, управление этому потоку не перейдет в отличие от обычного цикла, который будет бесконечно крутиться и съедать ресурсы
There was a problem hiding this comment.
Интересное решение. И по корректности работы не уступает CancellationTokenSource.
Однако в скорости проигрывает из-за volatile, и учитывая, что эти проверки будут постоянно проводиться в циклах потоков и не только, лучше классический вариант использовать с CancellationTokenSource
| /// <typeparam name="TResult">Func return type</typeparam> | ||
| /// <returns>I my task instance of one added to pool</returns> | ||
| public IMyTask<TResult> Submit<TResult>(Func<TResult> func) | ||
| { |
There was a problem hiding this comment.
Здесь не хватает проверки _isTerminated, из-за чего можем одним из потоков добавить задачу после вызова Shutdown, противоречие с условием.
Более того, надо лочиться по очереди задач пула здесь и в Shutdown, чтобы между ними гонок не возникало
There was a problem hiding this comment.
Закладывается ли в поведение объектов MyTask возможность по нескольку раз вызывать метод Execute? Как-будто бы нет, учитывая, что этот метод и в интерфейс не выносится.
Так что lock (и volatile-операции, кстати, несколько потоков же Execute тоже не вызывают) можно убрать, а то сейчас больше похоже на реализацию многопоточного lazy :)
There was a problem hiding this comment.
В классе сначала свойства, потом методы располагаются
There was a problem hiding this comment.
Необычно, но где гарантия, что эти потоки именно в пуле, а не созданы где-то в стороне (например, в одном из тестов).
Чтобы проверить число потоков именно в пуле можно, например, создать n задач с waitOne, проверить, что все потоки заняты (выполняют задачу), добавить n+1, проверить, что очередь задач не пуста (это, конечно, потребует добавления пары свойств в класс пула)
| System.Diagnostics.Process.GetCurrentProcess().Refresh(); | ||
| Assert.That(System.Diagnostics.Process.GetCurrentProcess().Threads, Has.Count.EqualTo(_procAmount + 4)); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Не хватает тестов на конкурентный доступ к методам пула из нескольких потоков: submit задач из разных потоков, Submit и Shutdown одновременно, ContinueWith и Shutdown
| Assert.That(secTask.IsCompleted, Is.True); | ||
| }); | ||
|
|
||
| _pool!.Submit(TestFunc); |
There was a problem hiding this comment.
Это не должно быть возможно после Shutdown по условию
| System.Diagnostics.Process.GetCurrentProcess().Refresh(); | ||
| Assert.That(System.Diagnostics.Process.GetCurrentProcess().Threads, Has.Count.EqualTo(_procAmount + 4)); | ||
| } | ||
| } No newline at end of file |
| @@ -0,0 +1,8 @@ | |||
| namespace MyThreadPool; | |||
|
|
|||
| public class MyThreadCreationException : Exception | |||
| var task = new MyTask<TResult>(func, this); | ||
| lock (this._cancellation) | ||
| { | ||
| if (!this._cancellation.IsCancellationRequested) |
There was a problem hiding this comment.
А если нет, нам возвращают задачу, которую никто не собирается делать. Обращаемся к её свойству Result — дедлок.
| } | ||
|
|
||
| public TResult Result() | ||
| { |
There was a problem hiding this comment.
Э нет, так нельзя. Надо проектировать API не исходя из того, чья будет в чём вина, а чтобы его удобно было использовать. Потенциальный дедлок — это вообще ни разу не "удобно использовать".
| this.IsCompleted = true; | ||
| this._completeEvent.Set(); | ||
| } | ||
| lock(this._nextTasks) |
There was a problem hiding this comment.
| lock(this._nextTasks) | |
| lock (this._nextTasks) |
| public TResult Result => | ||
| this._completeEvent.WaitOne() && this._threwException ? throw this._exception : this._result!; | ||
|
|
||
|
|
| } | ||
| public IMyTask<TNewResult> ContinueWith<TNewResult>(Func<TResult, TNewResult> nextDelegate) |
There was a problem hiding this comment.
А тут наоборот пустой строчки не хватает
| if (this.IsCompleted) | ||
| { | ||
| return this._threadPool.Submit(() => nextDelegate(this._result!)); | ||
| Console.WriteLine("Added after Completed"); |
There was a problem hiding this comment.
Тут даже CI говорит, что недостижимый код. Хоть он и, видимо, для отладки.
| { | ||
| if (!this._threadPool._cancellation.IsCancellationRequested) | ||
| { | ||
| this._nextTasks.Add(nextTask.Execute); |
There was a problem hiding this comment.
Вроде как дедлок, из-за которого не проходят тесты, тут — мы continuation ставим на пул, но не говорим пулу, что ему свалилась новая задача, так что все рабочие потоки сидят и ждут пока семафор откроется. А тест ждёт Result у continuation-а. Иногда ему везёт и ContinueWith отрабатывает, когда задача уже посчиталась, тогда тест проходит.
3 participants
No description provided.