Безопасен ли класс для многопоточности?

Question

[Acaunt]

Создаю класс для вывода информации о работе программы в консоль.

Пытался сохранить привычный синтаксис:
std::cout << "Hello World" << std::endl;

Также передаю информацию в очередь на вывод, чтобы при многопоточной работе не было подобной ситуации:

Start Start foo1
foo2
work work

Пока что пришёл к такому варианту:

...

#include <iostream>
#include <sstream>
#include <thread>
#include <mutex>
#include <queue>
using namespace std;

class Logger {
    class Message {
    private:
        ostringstream oss;
        
    public:
        Message() = default;
        Message(Message&&) = default;
        Message(const Message&) = delete;
        
    public:
        template<class Type>
        Message& operator<<(const Type& text) {
            oss << text;
            return *this;
        }
        
        Message& operator<<(ostream& (*manip)(ostream&)) {
            if (static_cast<ostream& (*)(ostream&)>(endl) == manip) {
                Logger& logg = Logger::get_instance();
                {
                    lock_guard<mutex> lock(logg.mtx);
                    logg.messages.push(oss.str());
                }
                oss.str("");
                oss.flush();
                oss.clear();
            }
            else {
                oss << manip;
            }
            
            return *this;
        }
    };
    
private:
    queue<string> messages;
    bool is_work;
    thread printer;
    mutex mtx;
    
private:
    Logger()
        : is_work(true)
        , printer(&Logger::print, this) {
    }
    
    ~Logger() {
        is_work = false;
        printer.join();
    }
    
public:
    Logger(Logger&&) = delete;
    Logger(const Logger&&) = delete;
    
public:
    static Logger& get_instance() {
        static Logger instace;
        return instace;
    }
    
private:
    void print() {
        while(is_work == true) {
            while (messages.empty() == false) {
                cout << messages.front() << endl;
                messages.pop();
            }
        }
    }
    
public:
    template<class Type>
    Message operator<<(const Type& text) {
        return move(Message() << text);
    }
    
    Message operator<<(ostream& (*manip)(ostream&)) {
        return move(Message() << manip);
    }
};

Logger& logg = Logger::get_instance();

// Симуляция многопоточной работы
static int j = 0;

int main() {
    for (size_t i = 0; i < 10; ++i) {
        thread th1([]() {
            logg << j++ << " Hello" << endl;
        });
        
        th1.detach();
        
        thread th2([]() {
            logg << j++ << " World" << endl;
        });
        
        th2.detach();
    }
    
    for(;;);
    
    return 0;
}

Порядок вывода информации не особо важен. Главное чтобы выводил цельные сообщения.

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

Безопасен ли будет такой класс при многопоточной работе? И какие ещё есть способы обезопасить процесс работы?

Answer 1

[res2001]

Не безопасен.
Вы защитили мьютексом только запись в очередь (push), но не чтение из нее (pop). Так что очередь сломается рано или поздно.
Вообще тут лучше бы подошла какая-то специализированная структура со встроенной поддержкой многопоточности, а не стандартные контейнеры.
Можно использовать кольцевой буфер или очередь майкла-скотта. В booste, на сколько помню, есть и то и другое.
Но для начала сгодится и такой вариант.

Кроме того для вывода на экран в Линукс можно учитывать тот факт, что ОС обеспечивает атомарную запись в консоль для буфера менее PIPE_BUF байт. https://linux.die.net/man/7/pipe
Думаю в винде то же есть похожая гарантия, но это не точно.

Кроме того, операция << для логгирования не очень подходит, т.к. вынуждает использовать конструкции типа:

oss << "Error: " << strerror(err) << "(" << err << ")";

для вывода 1 строки. А это уже не одна запись в очередь, а несколько. Так что защита мьютексом тут не будет гарантией вывода целостной строки.
Для логгера больше подойдет вариант типа std::printf, когда в один вызов передается вся информация для генерации целой строки.

Comments

[Acaunt]

Я чтение отдал отдельному потоку, который будет работать на протяжении всей работы. Разве это не обезопасит его?

[res2001]

Acaunt, Нет конечно. Ведь в этом случае возможно одновременное выполнение push и pop - и все, с большой вероятностью очередь тут и загнется. А если повезет и она не сломается на первом же случае, то сломается на следующем.
Все стандартные структуры не потокобезопасны, т.е. все операции с таким контейнером не потокобезопасны.
Так что защищать нужно вообще все манипуляции с очередью, кроме тех случаев когда вы точно уверены, что тут будет только один поток (например вызов конструктора).

[Acaunt]

res2001, кстати про оператор << вы возможно не заметили, но мой класс при получении оператора создаёт вспомогательный объект Message, который сначала собирает полную строку, а при получении std::endl он уже отправляет всю информацию в очередь очищаяя внутреннюю информацию.

Либо я вас не понял.

[res2001]

Acaunt, Да, что-то не допонял этот момент. Хорошо, тогда это снимается.