​​Разбор ревью
#новичкам

Большое спасибо всем участникам ревью, которые проявили активность под предыщущим постом. Всем и каждому посылаем лучи благодарности!

Было непросто выбрать самый эффективный по критике комментарий, потому что некоторые люди предлагали странные решения. В итоге, мы выбрали @seweeex и вот его коммент. Давайте похлопаем ему 👏👏👏.

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

Поехали разбирать.

🔞 Зачем-то в очереди хранятся сырые указатели. Смысла в этом особого нет, кроме как подложить себе свинью на будущее и поджечь жёпы комментаторов. Тут даже умные указатели не нужны, зачем дополнительные аллокации? В очереди можно хранить сами объекты и никаких проблем с менеджментом памяти не будет.

🔞 Использование сырых указателей приводит например к тому, что при вылете исключения из метода push, произойдет утечка памяти. Да, элементов мы закидываем в очередь немного, но концептуально проблема есть. Решается это опять же через хранение обычных объектов.

🔞 Слон в посудной лавке здесь - это конечно отсутствие синхронизации на очереди. Это в принципе ub и дальше не о чем говорить. Нужна не стандартная, а потокобезопасная очередь.

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

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

🔞 Бесконечный цикл в ресивере выглядит не очень. Если можно не писать бесконечных циклов и не вставлять брейки, то лучше этого не делать. break и continue усложняют понимание кода. Лучше перенести забирание элемента из очереди прям в шапку цикла.

🔞 Гонка на потоконебезопасном логировании. Нужен мьютекс, чтобы сообщения не интерферировали.

По сути все. Главное изменение - вынесение блокирующей потокобезопасной очереди в отдельный шаблонный класс, который хранит объекты типа Т. С шаблонами можно долго играться и далеко зайти, поэтому приведем самую простую реализацию, которая справляется со своими задачами в данном кейсе, но может быть улучшена для корректной работы с самыми разными типами:

#include <queue>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <memory>
#include <optional>
#include <print>

struct Message {
    int data;
};

template<typename T>
class ThreadSafeQueue {
public:
    void push(T msg) {
        {
            std::lock_guard lock(mutex_);
            queue_.push(std::move(msg));
        }
        cv_.notify_one();
    }

    std::optional<T> pop() {
        std::unique_lock lock(mutex_);
        cv_.wait(lock, [this] { return !queue_.empty() || stopped_; });
        
        if (stopped_ && queue_.empty()) {
            return std::nullopt;
        }

        auto msg = std::move(queue_.front());
        queue_.pop();
        return msg;
    }

    void stop_receive() {
        stopped_ = true;
        cv_.notify_all();
    }

private:
    std::queue<T> queue_;
    std::mutex mutex_;
    std::condition_variable cv_;
    std::atomic<bool> stopped_ = false;
};

void println(const std::string& str) {
    static std::mutex mtx;
    std::lock_guard lock(mtx);
    std::cout << str << std::endl;
}

void sender(ThreadSafeQueue<Message>& msgQueue) {
    for (int i = 0; i < 20; ++i) {
        auto msg = Message(i); 
        println("Sent: " + std::to_string(msg.data));
        msgQueue.push(std::move(msg));
    }
    msgQueue.stop_receive();
}

void receiver(ThreadSafeQueue<Message>& msgQueue) {
    while (auto msg = msgQueue.pop()) {        
        println("Received: " + std::to_string(msg.value().data));
    }
}

int main() {
    ThreadSafeQueue<Message> msgQueue;
    std::thread t1(sender, std::ref(msgQueue));
    std::thread t2(receiver, std::ref(msgQueue));

    t1.join();
    t2.join();
}

Пишите свои дополнения, если что-то забыли.

Make things better. Stay cool.