3. MyThreadPool - #3
Conversation
| @@ -0,0 +1,103 @@ | |||
| // <copyright file="MyThreadPoolTests.cs" company="PlaceholderCompany"> | |||
| // Copyright (c) PlaceholderCompany. All rights reserved. | |||
There was a problem hiding this comment.
Надо зарегистрировать ООО "PlaceholderCompany" и жить на доходы от патентного троллинга.
| public void Constructor_WithZeroThreads_Should_ThrowArgumentException() | ||
| { | ||
| Assert.Throws<ArgumentException>(() => { _ = new MyThreadPool(0); }); | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| [Test] | ||
| public void Dispose_Should_CallShutdown() | ||
| { | ||
| var pool = new MyThreadPool(2); |
There was a problem hiding this comment.
Я бы это в SetUp вынес, чтобы кучу раз не писать.
|
|
||
| Assert.Throws<InvalidOperationException>(() => pool.Submit(() => 42)); | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| public interface IMyTask<out TResult> | ||
| { | ||
| /// <summary> | ||
| /// Gets a value indicating whether it gets a value true if the task is completed. If the result is not yet ready, it returns false. |
There was a problem hiding this comment.
"Gets a value indicating whether it gets a value true if the task is completed" — 🤔
| { | ||
| ArgumentNullException.ThrowIfNull(newFunction); | ||
|
|
||
| return this.threadPool.Submit(ContinuationFunction); |
There was a problem hiding this comment.
Так в пул будут ставиться ещё не готовые к работе задачи, которые заблочатся на Result "родительской" задачи. Так вполне может оказаться, что все потоки пула ждут, пока закончится одна задача, и ничего не делают, тогда как в очереди куча быстрых и готовых к запуску задач, которые просто некому делать. Так что ставить в пул продолжения надо только когда "родительская" задача закончена.
|
|
||
| TNewResult ContinuationFunction() | ||
| { | ||
| var sourceResult = this.Result; |
There was a problem hiding this comment.
Если родительская задача завершилась с исключением, тут беда будет.
| if (threadCount <= 0) | ||
| { | ||
| throw new ArgumentException("Thread count must be positive.", nameof(threadCount)); | ||
| } |
There was a problem hiding this comment.
Используйте ArgumentOutOfRangeException.ThrowIfLessOrEqual.
| throw new InvalidOperationException("ThreadPool is shutting down."); | ||
| } | ||
|
|
||
| var task = new MyTask<TResult>(function, this); |
There was a problem hiding this comment.
Прямо вот здесь this.isDisposed может уже быть true. taskQueue.Add бросит исключение, так что вроде как ничего не сломается, но но несинхронизированная проверка разделяемой между потоками переменной всегда вызывает ощущение лёгкого удивления.
| return; | ||
| } | ||
|
|
||
| this.isDisposed = true; |
There was a problem hiding this comment.
Тут тоже, никто не мешает вызвать Shutdown из двух потоков одновременно.
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
Уже не актуально, но вроде как теперь почти всё правильно
| { | ||
| try | ||
| { | ||
| var task = this.poolWithThreeThreads.Submit(() => |
There was a problem hiding this comment.
Стоило бы для пущей убедительности стартовать Submit по максимуму одновременно, добавив сюда ManualResetEvent.Wait, и выставлять его перед Assert-ом. А то тут не исключено, что все Submit-ы будут делаться строго последовательно и вообще в одном потоке — задача-то короткая, легко укладывается в один квант времени.
| this.isCompleted = true; | ||
| this.completionEvent.Set(); | ||
| this.StartContinuations(); | ||
| } |
There was a problem hiding this comment.
Ещё бы занулить function, чтобы сборщик мусора мог её собрать (со всем её замыканием, которое может включать в себя полсистемы).
| public void Start() | ||
| { | ||
| _ = this.lazyTask.Value; | ||
| } |
| => this.lazyTask.Value.Result; | ||
|
|
||
| public IMyTask<TNextResult> ContinueWith<TNextResult>(Func<TNewResult, TNextResult> function) | ||
| => this.lazyTask.Value.ContinueWith(function); |
There was a problem hiding this comment.
Хм, так вызов ContinueWith у ещё не посчитавшейся задачи приведёт к её внезапной и несвоевременной постановке на тредпул, как я понял. Тут Lazy — некий оверкилл на самом деле, MyTask и так почти Lazy, можно было бы требуемую функциональность без таких странных побочных эффектов реализовать вручную (или обойтись обычным MyTask на самом деле). Правда, с Lazy не надо разбираться, в какой поток Submit выкинет исключение, это большой плюс такого подхода.
| private readonly List<Thread> threads; | ||
| private readonly CancellationTokenSource cancellationTokenSource; | ||
| private volatile bool isDisposed; | ||
| private readonly Lock shutdownLock = new(); |
There was a problem hiding this comment.
Тут стоило поправить предупреждения от StyleCop.
3 participants
No description provided.