у кого время есть, покритикуйте?
От: nikkit  
Дата: 21.04.23 10:31
Оценка:
в общем так получилось, что в основном приходилось только бизнес логику ковырять. многопоточности касался очень немного. в ближайшее время данный пробел устраню.
в общем выставляю простейшую программу:
— винформс. форма с тремя кнопками: старт, пауза, стоп
— при нажатии старт запускается вычислительный процесс (тут у меня просто берет значение из поля ввода (дату), увеличивает ее на день, помещает новую дату в другой контрол и включает задержку). процесс выполняется итеративно, в фоне.
— при нажатии на паузу, выполнение процесса приостанавливается, при повторном нажатии возобновляется;
— при нажатии на стоп, процесс останавливается
В общем любой конструктив: критика, советы, вопросы по выбору такой реализации и т.п. категорически приветствуется.

using System;
using System.Threading;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace WorkWithMultithreading
{
    public partial class MainForm : Form
    {
        private const int timeout = 5000;

        private bool isPaused = false;
        private bool isStopped = false;
        private SynchronizationContext synchronizationContext;
        private CancellationTokenSource cancelTokenSource;
        private Task workingTask;
        public MainForm()
        {
            InitializeComponent();
            synchronizationContext = SynchronizationContext.Current;
        }

        private void btStart_Click(object sender, System.EventArgs e)
        {
            if (workingTask?.IsCompleted??true)
            {
                cancelTokenSource = new CancellationTokenSource();
                isStopped = false;
                workingTask = Task.Run(async () =>
                {
                    while (!isStopped && !cancelTokenSource.IsCancellationRequested)
                    {
                        if (!isPaused)
                        {
                            DateTime dt;

                            synchronizationContext.Send((_) =>
                            {
                                dt = dtpInputDate.Value.Date.AddDays(1);
                                edtResult.Text = dt.ToString();
                            }, null);
                        }

                        await Task.Delay(timeout);
                    }
                }, cancelTokenSource.Token);
            }
        }

        private void btPause_Click(object sender, System.EventArgs e)
        {
            if (!isStopped)
            {
                isPaused = !isPaused;
                btPause.Text = isPaused ? "Continue" : "Pause";
            }
        }

        private void btStop_Click(object sender, System.EventArgs e)
        {
            isStopped = true;
            edtResult.Text = string.Empty;
            cancelTokenSource.Cancel();
        }

        private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
        {
            isStopped = true;
            workingTask?.Wait();
        }
    }
}
Re: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 11:04
Оценка: 9 (2) +1
Здравствуйте, nikkit, Вы писали:
N>В общем любой конструктив: критика, советы, вопросы по выбору такой реализации и т.п. категорически приветствуется.

0) Зачем создавать таску и сию секунду отправлять в контекст GUI!?
Таски с такой логикой бессмысленны — разве что сожрать больше памяти и процессора хочется.


1)
а)
N>
N>        private bool isPaused = false;
N>        private bool isStopped = false;
N>


volatile

б)
N>
N>                            synchronizationContext.Send((_) =>
N>                            {
N>                                dt = dtpInputDate.Value.Date.AddDays(1);
N>                                edtResult.Text = dt.ToString();
N>                            }, null);
N>                        }

N>


Лучше вот так:
//в гуёвом треде
uiScheduler = TaskScheduler.FromCurrentSynchronizationContext();
....
//внутри таски
task = Task.Factory.StartNew(.........CancellationToken, TaskCreationOptions, uiScheduler )
task.Wait()


Обоснование уже не помню: с synchronizationContext в чистом виде какие-то проблемы раньше были.

в)
Scheduler желательно указывать всегда самостоятельно: TaskScheduler.Current может не равно TaskScheduler.Default — огребёшь проблем.

г) Лучше не делать таску внутри таски — лучше
                   .ContinueWith(task => {
                    if (task.Exception != null) {
                        AggregateException _exc = task.Exception.Flatten();
                        ShowError(_exc.InnerException);
            return;
                    }

           ....
            //тут твоя логика
                }, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, GetUITaskScheduler());


2) await Task.Delay(timeout);
Насчёт вот этого не уверен. Почитал — оно вообще создано для другого. Оно таску создаёт, которой можно ContinueWith(...).
Здесь лучше Thread.Sleep(0)
Всё сказанное выше — личное мнение, если не указано обратное.
Re: у кого время есть, покритикуйте?
От: Janus Россия  
Дата: 21.04.23 11:18
Оценка:
Здравствуйте, nikkit, Вы писали:

N>в общем так получилось, что в основном приходилось только бизнес логику ковырять. многопоточности касался очень немного. в ближайшее время данный пробел устраню.

N>в общем выставляю простейшую программу:
N>- винформс. форма с тремя кнопками: старт, пауза, стоп
N>- при нажатии старт запускается вычислительный процесс (тут у меня просто берет значение из поля ввода (дату), увеличивает ее на день, помещает новую дату в другой контрол и включает задержку). процесс выполняется итеративно, в фоне.
N>- при нажатии на паузу, выполнение процесса приостанавливается, при повторном нажатии возобновляется;
N>- при нажатии на стоп, процесс останавливается
N>В общем любой конструктив: критика, советы, вопросы по выбору такой реализации и т.п. категорически приветствуется.


А backgroundworker для этой задачи не подойдет ?
... Хорошо уметь читать между строк. Это иногда
приносит большую пользу
Re[2]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 11:27
Оценка:
Здравствуйте, Janus, Вы писали:

J>А backgroundworker для этой задачи не подойдет ?


Подойдёт. Вот только это компонент кладущийся на форму — он не для общего случая. А логику часто делают за пределами форм, или переносят её за пределы в процессе рефакторинга.
Кроме того, backgroundworker не умеет джойнится с другими такими же — он из-за этого на порядок менее удобен чем таски.
Всё сказанное выше — личное мнение, если не указано обратное.
Re[2]: у кого время есть, покритикуйте?
От: nikkit  
Дата: 21.04.23 11:36
Оценка:
Спасибо за разбор.

Ф>0) Зачем создавать таску и сию секунду отправлять в контекст GUI!?

Ф>Таски с такой логикой бессмысленны — разве что сожрать больше памяти и процессора хочется.

а какая альтернатива?

N>>
N>>        private bool isPaused = false;
N>>        private bool isStopped = false;
N>>


Ф>volatile


можно подробнее, что имеешь в виду?

Почитаю по остальным моментам, задам еще вопросы, где непонятно.
Re[3]: у кого время есть, покритикуйте?
От: Janus Россия  
Дата: 21.04.23 11:37
Оценка:
Здравствуйте, Философ, Вы писали:

J>>А backgroundworker для этой задачи не подойдет ?


Ф>Подойдёт. Вот только это компонент кладущийся на форму — он не для общего случая. А логику часто делают за пределами форм, или переносят её за пределы в процессе рефакторинга.


backgroundworker не обязательно класть на форму .
Бонусом backgroundworker будет прогресс из коробки

Ф>Кроме того, backgroundworker не умеет джойнится с другими такими же — он из-за этого на порядок менее удобен чем таски.

Этого пункта нет в описании задачи
... Хорошо уметь читать между строк. Это иногда
приносит большую пользу
Re[3]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 11:44
Оценка:
Здравствуйте, nikkit, Вы писали:

N>Спасибо за разбор.


Ф>>0) Зачем создавать таску и сию секунду отправлять в контекст GUI!?

Ф>>Таски с такой логикой бессмысленны — разве что сожрать больше памяти и процессора хочется.

N>а какая альтернатива?


Стакой логикой — без всяких тасок и потоков, просто твоя логика в цикле и там же Application.DoEvents () вместо всяких Thread.Sleep();


Ф>>volatile

N>можно подробнее, что имеешь в виду?

        private volatile bool isPaused = false;
        private volatile bool isStopped = false;


У тебя эти поля в разных потоках используются.
Всё сказанное выше — личное мнение, если не указано обратное.
Re[4]: у кого время есть, покритикуйте?
От: nikkit  
Дата: 21.04.23 11:47
Оценка:
Ф>У тебя эти поля в разных потоках используются.

нужно им делать lock?
Re[5]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 11:51
Оценка:
Здравствуйте, nikkit, Вы писали:


Ф>>У тебя эти поля в разных потоках используются.


N>нужно им делать lock?


Нет: bool — атомарное значение. Все операции с ним всегда атомарны.
Если за один раз будешь анализировать несколько атомарных значений, то тогда будет нужен.
Всё сказанное выше — личное мнение, если не указано обратное.
Отредактировано 21.04.2023 11:52 Философ . Предыдущая версия .
Re[6]: у кого время есть, покритикуйте?
От: nikkit  
Дата: 21.04.23 12:02
Оценка:
Ф>Нет: bool — атомарное значение. Все операции с ним всегда атомарны.
Ф>Если за один раз будешь анализировать несколько атомарных значений, то тогда будет нужен.

как тогда пофиксить данный недостаток?
Re: у кого время есть, покритикуйте?
От: _FRED_ Черногория
Дата: 21.04.23 12:05
Оценка: 2 (1) +1
Здравствуйте, nikkit, Вы писали:

N>В общем любой конструктив: критика, советы, вопросы по выбору такой реализации и т.п. категорически приветствуется.


Было б удачнее залить это на гитхаб и сделать там пулл реквест: удобнее комментить и можно, кому интересно, собрать и даже предложить что-то.

Я б для начала задумался бы, где диспозится `cancelTokenSource` (ведь оно `IDisposable`).

Не совсем ясно, зачем нужен `isStopped` флаг, вроде без него можно обойтись, ведь есть `cancelTokenSource` — кансельнул, значит остановил.
Help will always be given at Hogwarts to those who ask for it.
Re[7]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 12:09
Оценка: +1
Здравствуйте, nikkit, Вы писали:

N>как тогда пофиксить данный недостаток?


Какой недостаток? Не понял.
Тебе нужно написать пример кода, где обязательно нужен lock? Он далеко не везде нужен: люди часто обходятся Interlocked-функциями.
Всё сказанное выше — личное мнение, если не указано обратное.
Re[8]: у кого время есть, покритикуйте?
От: nikkit  
Дата: 21.04.23 12:28
Оценка:
Ф>Какой недостаток? Не понял.
Ф>Тебе нужно написать пример кода, где обязательно нужен lock? Он далеко не везде нужен: люди часто обходятся Interlocked-функциями.

ну ты написал в коментах к этой части volatile. имеешь в виду, просто модификатор поставить:

private volatile bool isPaused = false;
Re[2]: у кого время есть, покритикуйте?
От: _FRED_ Черногория
Дата: 21.04.23 12:30
Оценка:
Здравствуйте, Философ, Вы писали:

N>>        private bool isPaused = false;
N>>        private bool isStopped = false;

Ф>volatile

Кажется, это не очень хороший совет человеку, который только начинает разбираться со всем этим
Help will always be given at Hogwarts to those who ask for it.
Re[9]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 12:31
Оценка:
Здравствуйте, nikkit, Вы писали:

N>ну ты написал в коментах к этой части volatile. имеешь в виду, просто модификатор поставить:


N>
N>private volatile bool isPaused = false;
N>


да
Всё сказанное выше — личное мнение, если не указано обратное.
Re[3]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 12:34
Оценка:
Здравствуйте, _FRED_, Вы писали:

Ф>>volatile


_FR>Кажется, это не очень хороший совет человеку, который только начинает разбираться со всем этим


ну хз, в документации мелкомягких вполне себе написано зачем оно

Я думаю, если человек написал всё это то уж глянуть что это слово означает точно сможет:

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

Всё сказанное выше — личное мнение, если не указано обратное.
Re[4]: у кого время есть, покритикуйте?
От: _FRED_ Черногория
Дата: 21.04.23 12:44
Оценка:
Здравствуйте, Философ, Вы писали:

Ф>>>volatile

_FR>>Кажется, это не очень хороший совет человеку, который только начинает разбираться со всем этим
Ф>ну хз, в документации мелкомягких вполне себе написано зачем оно

Это здорово, что вам так всё ясно с этим. Но у других может быть не так.

Ф>Я думаю, если человек написал всё это то уж глянуть что это слово означает точно сможет:

Ф>Ключевое слово volatile означает, что поле может изменить несколько потоков, выполняемых одновременно. Компилятор, среда выполнения или даже аппаратное обеспечение могут изменять порядок операций чтения и записи в расположения в памяти для повышения производительности. Поля, которые объявлены volatile, исключаются из некоторых типов оптимизации.


Я перечитал ещё раз код в стартовом сообщении и не увидел, чтобы хоть одно из полей, объявление которых вы рекомендовали изменить подобным образом, менялось бы из разных потоков
Help will always be given at Hogwarts to those who ask for it.
Re[5]: у кого время есть, покритикуйте?
От: Философ Ад http://vk.com/id10256428
Дата: 21.04.23 12:52
Оценка:
Здравствуйте, _FRED_, Вы писали:

_FR>Это здорово, что вам так всё ясно с этим. Но у других может быть не так.

_FR>...
_FR>Я перечитал ещё раз код в стартовом сообщении и не увидел, чтобы хоть одно из полей, объявление которых вы рекомендовали изменить подобным образом, менялось бы из разных потоков

Намёк понял — буду иметь ввиду. Поторопился.
Всё сказанное выше — личное мнение, если не указано обратное.
Re[6]: у кого время есть, покритикуйте?
От: _FRED_ Черногория
Дата: 21.04.23 13:37
Оценка:
Здравствуйте, Философ, Вы писали:

_FR>>Это здорово, что вам так всё ясно с этим. Но у других может быть не так.

_FR>>...
_FR>>Я перечитал ещё раз код в стартовом сообщении и не увидел, чтобы хоть одно из полей, объявление которых вы рекомендовали изменить подобным образом, менялось бы из разных потоков

Ф>Намёк понял — буду иметь ввиду. Поторопился.


Чтобы было совсем уж ясно: "не увидел, чтобы хоть одно из полей… менялось бы из разных потоков" тут не причём, просто

Frankly, I discourage you from ever making a volatile field. Volatile fields are a sign that you are doing something downright crazy: you’re attempting to read and write the same value on two different threads without putting a lock in place. Locks guarantee that memory read or modified inside the lock is observed to be consistent, locks guarantee that only one thread accesses a given hunk of memory at a time, and so on. The number of situations in which a lock is too slow is very small, and the probability that you are going to get the code wrong because you don’t understand the exact memory model is very large. I don’t attempt to write any low-lock code except for the most trivial usages of Interlocked operations. I leave the usage of volatile to real experts.

  Гугл перевод


Честно говоря, я не рекомендую вам когда-либо создавать изменчивое поле. Неустойчивые поля — это признак того, что вы делаете что-то совершенно безумное: вы пытаетесь прочитать и записать одно и то же значение в два разных потока, не устанавливая блокировку. Блокировки гарантируют, что память, прочитанная или измененная внутри блокировки, будет наблюдаться как непротиворечивая, блокировки гарантируют, что только один поток получает доступ к данному фрагменту памяти в каждый момент времени и так далее. Количество ситуаций, в которых блокировка работает слишком медленно, очень мало, а вероятность того, что вы наберете код неправильно, потому что не понимаете точную модель памяти, очень велика. Я не пытаюсь писать какой-либо код низкой блокировки, за исключением самых тривиальных случаев использования операций Interlocked. Я оставляю использование volatile для настоящих экспертов.
Help will always be given at Hogwarts to those who ask for it.
Re: у кого время есть, покритикуйте?
От: _FRED_ Черногория
Дата: 21.04.23 13:56
Оценка: 2 (1) +2
Здравствуйте, nikkit, Вы писали:

N>в общем так получилось, что в основном приходилось только бизнес логику ковырять. многопоточности касался очень немного. в ближайшее время данный пробел устраню.


И, простите, ещё пара слов: если вы хотите разобраться с "многопоточностью", то не надо пока брать async. Вынесите задачу в отдельный поток или даже в ThreadPool.QueueUserWorkItem, замените булевые флаги на EventWaitHandle и "устраняйте". Асинхронность, вообще говоря, это не совсем то же самое, что и многопоточность. Из-за этого что-то может усваиваться не так правильно, как, возможно, вам хотелось бы.
Help will always be given at Hogwarts to those who ask for it.
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.