-
Notifications
You must be signed in to change notification settings - Fork 0
Lazyy #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Lazyy #2
Conversation
yurii-litvinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Всё несколько сложнее, чем Вы думаете :) Там ещё где-то надо volatile
Lazyy/Lazyy.Test/MultiThreadTest.cs
Outdated
| threads[i] = new Thread(() => lazyMulti.Get()); | ||
| } | ||
|
|
||
| foreach (var thread in threads) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| foreach (var thread in threads) | |
| foreach (var thread in threads) |
Lazyy/Lazyy.Test/MultiThreadTest.cs
Outdated
| { | ||
| thread.Start(); | ||
| } | ||
| foreach (var thread in threads) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| foreach (var thread in threads) | |
| foreach (var thread in threads) |
Lazyy/Lazyy/ILazy.cs
Outdated
| @@ -0,0 +1,7 @@ | |||
| namespace Lazyy | |||
| { | |||
| public interface ILazy<T> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public interface ILazy<T> | |
| public interface ILazy<out T> |
| { | ||
| public interface ILazy<T> | ||
| { | ||
| public T Get(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Lazyy/Lazyy/LazyFactory.cs
Outdated
| /// </summary> | ||
| public class LazyFactory<T> | ||
| { | ||
| public static LazySingle<T> CreateSingleLazy(Func<T> supplier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
И даже тут комментарии нужны
| lock (this._lockObject) | ||
| { | ||
| _isGenerate = true; | ||
| _value = _supplier(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Lazyy/Lazyy/LazySingle.cs
Outdated
| namespace Lazyy | ||
| { | ||
| /// <summary> | ||
| /// реаилизация однопоточного режима |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// реаилизация однопоточного режима | |
| /// реализация однопоточного режима |
Lazyy/Lazyy/LazySingle.cs
Outdated
| private Func<T> _supplier; | ||
| private bool _isGenerate = false; | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lazyy/Lazyy/LazyFactory.cs
Outdated
| /// <summary> | ||
| /// создает обьекты для работы либо в однопоточном, лио многопоточном режиме | ||
| /// </summary> | ||
| public class LazyFactory<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public class LazyFactory<T> | |
| public static class LazyFactory<T> |
И параметр-тип ему не нужен, лучше методы генериками сделать. Тогда не надо будет указывать параметр-тип при вызове.
Lazyy/Lazyy/Program.cs
Outdated
| { | ||
| thread.Join(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lazyy/Lazyy.Test/SingleThreadTest.cs
Outdated
| Assert.Throws<NullReferenceException>(() => LazyFactory.CreateSingleLazy<int>(null)); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нигде не проверяется, что если supplier null, происходит что-то разумное, что если supplier возвращает null, то тоже
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо, а если supplier возвращает null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не поправлено
Lazyy/Lazyy/LazyFactory.cs
Outdated
| namespace Lazyy | ||
| { | ||
| /// <summary> | ||
| /// создает обьекты для работы либо в однопоточном, лио многопоточном режиме |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// создает обьекты для работы либо в однопоточном, лио многопоточном режиме | |
| /// создает обьекты для работы либо в однопоточном, либо в многопоточном режиме |
Lazyy/Lazyy/LazyMulti.cs
Outdated
| /// <summary> | ||
| /// создает обьект в многопоточном режиме | ||
| /// </summary> | ||
| public LazyMulti(Func<T> supplierNew) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Как-то странно параметр называется
| lock (this._lockObject) | ||
| { | ||
| _isGenerate = true; | ||
| _value = _supplier(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
yurii-litvinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не-а :) Почти правильно, но всё равно есть гонка, и у Вас стремительно заканчиваются попытки
Lazyy/Lazyy.Test/SingleThreadTest.cs
Outdated
| Assert.Throws<NullReferenceException>(() => LazyFactory.CreateSingleLazy<int>(null)); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо, а если supplier возвращает null?
Lazyy/Lazyy/LazyMulti.cs
Outdated
| namespace Lazyy | ||
| { | ||
| /// <summary> | ||
| /// реализация многопоточного режима |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну это не совсем правдивый комментарий. Это ведь не реализация pthreads или System.Threading :)
Lazyy/Lazyy/LazyMulti.cs
Outdated
|
|
||
| _value = _supplier(); | ||
| _supplier = null; | ||
| _isGenerate = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
И тут наносит удар volatile и модель памяти многоядерных процессоров... _isGenerate = true другие потоки могут увидеть до _value = _supplier();, что приведёт к возврату null и крешу вызывающего.
Lazyy/Lazyy/LazyMulti.cs
Outdated
| public class LazyMulti<T> : ILazy<T> | ||
| { | ||
| private T _value; | ||
| private bool _isGenerate = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_isGenerated?
yurii-litvinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Окей, осталось только унифицировать тесты (уже не в счёт попыток). Все варианты Lazy реализуют один интерфейс и должны обладать одинаковым поведением на базовых сценариях (на null, на supplier, который возвращает null, что они не запускают вычисление дважды). Значит, можно написать один набор тестов и скармливать ему Lazy через TestCaseData (или, точнее, скармливать тестам функции, которые производят Lazy, можно прямо методы фабики --- ведь нам надо из теста им supplier передать). Тогда получится, что есть общие тесты на все Lazy, и один тест на многопоточную --- что гонок нет. А на однопоточную Lazy специфические тесты вообще не нужны, получается.
Lazyy/Lazyy.Test/SingleThreadTest.cs
Outdated
| Assert.Throws<NullReferenceException>(() => LazyFactory.CreateSingleLazy<int>(null)); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не поправлено
yurii-litvinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ага, зачтена
No description provided.