Простой многопоточный FileLogger на C ++

Я новичок в C ++ и продолжаю следить за: Простым многопоточным FileLogger на C ++

Мне любопытно узнать, правильно ли я реализовал его идеи, есть ли дальнейшие улучшения в коде и правильно ли он (все еще).

FileLoggerThread.h

#pragma once
#include <string>
#include <fstream>
#include <mutex>
#include <deque>
#include <condition_variable>
#include <filesystem>

class FileLoggerThread
{
public:
    explicit FileLoggerThread(std::filesystem::path filePath);
    ~FileLoggerThread();                            // makes code more easily unit testable (unit tests can otherwise be in an infinite loop)

    void log(const std::string &msg);

    FileLoggerThread(const FileLoggerThread&) = delete;
    FileLoggerThread& operator=(const FileLoggerThread&) = delete;

private:
    void writeToFile();

    std::deque<std::string> mMessages;              // accessed by log (any thread) and writeToFile (the writer thread)
    std::ofstream mOutputFile;                      // only accessible by writeToFile
    
    // thread internal resources
    std::mutex mMessagesMutex;                      // lock/unlock this each time messages is pushed or popped
    std::condition_variable mThreadScheduler;       // log needs to wake up writeToFile
    bool mIsRunning;                                // if set to false then the writer thread stops
};

FileLoggerThread.cpp

#include "FileLoggerThread.h"
#include <thread>

FileLoggerThread::FileLoggerThread(std::filesystem::path filePath) : mOutputFile {filePath}, mIsRunning {true} {
    std::thread t(&FileLoggerThread::writeToFile, this);
    t.detach();
}

void FileLoggerThread::writeToFile() {
    while (mIsRunning) {
        std::deque<std::string> localMessages;
        std::unique_lock<std::mutex> writerLock {mMessagesMutex};
        mThreadScheduler.wait(writerLock, [&]{ return !mMessages.empty() || !mIsRunning;});
        localMessages.swap(mMessages);
        writerLock.unlock();

        for (const auto& message : localMessages) {
            mOutputFile << message << std::endl;
        }
    }
}

void FileLoggerThread::log(const std::string &msg) {
    std::unique_lock loggerLock(mMessagesMutex);
    mMessages.emplace_back(msg);
    loggerLock.unlock();
    mThreadScheduler.notify_one(); // done for performance reasons, see: https://youtu.be/F6Ipn7gCOsY?t=2205
}

FileLoggerThread::~FileLoggerThread() {
    mIsRunning = false;
    mThreadScheduler.notify_one();
}

• Не рекомендуется иметь полностью отделенную нить. Когда кто-то отключает поток, они обычно используют какую-то систему уведомлений, например future / обещание, чтобы гарантировать, что поток завершил использование ресурсов. Здесь вы, возможно, завершили работу регистратора файлов и были уничтожены, в то время как отключенный поток все еще обращается this ведущий к УБ. Вместо этого вы должны сохранить поток как часть регистратора и триггера. join в деструкторе.

• Вы не должны использовать std::endl; для каждого сообщения — промывка выполняется медленно. У вас определенно нет веских причин запускать его чаще, чем один раз в секунду. Просто убедитесь, что вы запускаете его примерно раз в секунду. std::condition_variable::wait_for/wait_until можно использовать для этого.

• Еще одна ошибка деструктора заключается в том, что вы изменяете mIsRunning без блокировки мьютекса. Технически это UB. На практике эта ошибка в очень редких случаях приводит к wait отсутствует уведомление и длительная вечность.

• В схеме, которую вы используете, я не понимаю, почему вы используете std::deque для хранения сообщений. Почему не просто std::vector? Это более эффективно, и вам не нужно создавать новые localMessages каждый раз — просто держите один постоянно существующим и очищайте его после каждого использования. Это экономит некоторые ненужные выделения памяти.

• Кроме того, однажды mIsRunning является false вам нужно убедиться, что все сообщения были написаны вместо того, чтобы выйти. Более того, вы не должны получать доступ mIsRunning в условии цикла while, так как он должен быть защищен мьютексом.

• Функция журнала void log(const std::string &msg); принимает строку и копирует ее в очередь. Это пустая трата. Вам лучше иметь две другие перегрузки: void log(std::string&& msg); и void log(std::string_view msg); с первым перемещением строки и ее содержимого для печати вместо ненужного копирования, в то время как последний является просто более общей формой const std::string&.

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