​​Результаты ревью

Всем участникам спасибо за развернутые ответы. Самый объемный комментарий с большим количеством правильных предположений относительно кода написал Alex D. Давайте ему все вместе похлопаем👏👏👏.

Также удивило, что некоторые предоставили прям свои исправленные версии кода. Спасибо большое за вовлеченность😌. Ваши примеры помогают новичкам по-другому посмотреть на задачу и увидеть альтернативные и более корректные версии ее решения.

Но мы не будем уж погружаться совсем далеко в дебри и не будет совсем переписывать кусок, чтобы в нем появлялись атомики, тредпулы, кондвары и бечконечный разбор очереди до graceful shutdown. Уже в одном из прошлых ревью разбирали, как может выглядеть более совершенная версия воркера для очереди. Все ваши комменты и примеры кода с более продвинутой обработкой в любом случае имеют смысл. Просто этот пример довольно учебный и планировал показывать вполне конкретные вещи, не претендуя на что-то сложное.

Но и на этом поле есть, где развернуться.

Итак, пойдем по в рандомном порядке:

💥 Очередь копируется в воркер потока, поэтому мы обрабатываем не просто копию очереди, а копии!. Каждый поток имеет свою копию. Передача по значению не была бы прям уж сильной проблемой, если бы был один воркер. А их тут 2. И вся работа будет делаться дважды. Это конечно не порядок. Меняем параметр функции на ссылку.

💥 Но если вы думаете, что вы так передадите потоку ссылку - вы ошибаетесь! На самом деле для всех объектов-аргументов потоковой функции во внутреннем сторадже нового потока создается их копия, чтобы не иметь дело с висячими ссылками. Если вы хотите передать истинную ссылку в поток, то надо воспользоваться std::ref.

💥 Нет join'ов у потоков. Надо их либо вызвать, либо использовать jthread из С++20.

💥 Одна из самых больших непоняток в коде для комментаторов - что за переменная lck? На самом деле автор кода немного с ним экспериментировал и просто допустил ошибку, не переименовав lck в mtx. Такое бывает, особенно с новичками. Особенно в проектах без CI.

💥 Если вы используете мьютекс, это не значит, что все автоматически становится потокобезопасным. В этом примере мьютекс никак не защищает критическую секцию! А как он может защитить, если он является локальной переменной. То есть каждый поток будет иметь свою копию этого мьютекса. Не порядок. Его надо сделать либо глобальным вообще, либо статической локальной переменной.

💥 Использование чистых вызовов lock и unlock на мьютексах является очень плохой практикой. В случае изначально пустой очереди или такого распределения расписания потоков, что очередь окажется пуста до входа в while, один из потоков никогда не отпустит лок.
Да и вот это засыпание после execute выглядит очень криво.
Нужно использовать RAII обертки, типа std::lock_guard.

💥 Но в нашем случае lock_guard будет плохим решением. Во-первых, не очень понятно, куда его вставлять. Изначальная проверка на пустоту тоже должна быть "обезопашена". Тогда надо ставить до while. Но в этом случае один из потоков будет выполнять всю работу целиком. А нам хотелось бы использовать преимущества многопоточки.

И еще один нюанс: у нас под локом выполняется довольно тяжелая функция, которая долго выполняется. И все это время другие потоки будут простаивать без дела. А если у нас со временем их станет больше? Еще больше простоя. А время - это деньги.

В общем, нехватает гибкости. Хочется иметь возможность отпускать замок на время, а потом опять его захватывать. Но и преимуществами RAII тоже хочется пользоваться.
Выход - std::unique_lock. Он позволяет делать и то, и то.

💥 Так как выполнение задачи теперь не под локом, то sleep в воркере не нужен, другие потоки имеют достаточно времени, чтобы захватить мьютекс.

💥 Неплохо бы обернуть выполнение задачи в блок try-catch. Так если вылетит исключения из выполнения одной задачи, мы можем как-то обработать эту ситуацию и пойти работать дальше, а не просто свалиться в std::terminate.
Пусть этот пример останется "учебным", но теперь хотя бы корректным.

ПРОДОЛЖЕНИЕ В КОММЕНТАРИЯХ

Fix your flaws. Stay cool.