Как правильно настроить взаимодействие от переменной объявленной в классе?

Question

[Андрей]

В данный момент написан абстрактный интерфейс класса:

class InstrumentInterface {
public:
	virtual ~InstrumentInterface() = default;
	virtual bool getLock() = 0;
	virtual std::string writeCommand(std::string*) = 0;
	virtual std::string readCommand() = 0;
protected:
	std::mutex interfaceControl;
};

От него определяю методы:

class InstrumentalServer : public InstrumentInterface
{
public:
	InstrumentalServer() = default;
	virtual ~InstrumentalServer();

	void initSocketServer();

	void addDevice(AbstractDevice*);

	virtual bool getLock() override;
	virtual std::string writeCommand(std::string*) override;
	virtual std::string readCommand() override;
private:
	SocketServer* sockServer;
	AbstractDevice* device;
};

Внуть класса SocketServer передаю this для возможности взаимодействия с AbstractDevice (будут еще 2 сервера и передавать AbstractDevice серверу бессмысленно, так как класс InstrumentalServer должен контролировать доступ к AbstractDevice ):

void InstrumentalServer::initSocketServer()
{
	sockServer = new SocketServer(this);
}

Внутри класса SocketServer в отдельном потоке сервера вызываю функции интерфейса:

try
{
	Interface->writeCommand(buf);
}
catch (const std::exception& ex)
{
	std::cerr << ex.what() << std::endl;
}

Насколько это правильно? Каким правильным способом я могу отдать данные в InstrumentalServer из SocketServer?
Спасибо за ответы!

Comments

[Евгений Шатунов]

Насколько это правильно?

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

Answer 1

[menkar3]

Отвечу как сам понял вопрос, может быть вообще мимо.
Основная проблема - сильная связность. У вас более низкоуровневый класс SocketServer зачем-то должен знать о существовании InstrumentInterface который им владеет.
Я бы предложил передавать серверу в конструкторе "что-то", куда он сможет писать полученные данные. Простой пример - InstrumentalServer ждет, пока не поступят данные от сервера, и пишет их в девайс. Простой пример, который работает вечно - сервер просыпается раз в секунду, записывает данные и засыпает. При появление данных InstrumentalServer просыпается, пишет их в девайс, и засыпает, снова ожидая входящие данные. Классическая многопоточная задачка же. Удалил методы из интерфейса.

struct MessageQueue
{
    std::condition_variable cv;
    std::mutex messMtx;
    std::queue<std::string> serverMessages;
};

struct AbstractDevice
{
    void processData(std::string message)
    {
        std::cout << message << std::endl;
    }
};

class SocketServer
{
public:
    SocketServer(MessageQueue& mess)
        : out(mess)
    {
        std::thread t(&SocketServer::processData, this);
        t.detach();
    }

    void processData()
    {
        for (;;)
        {
            try
            {
                std::lock_guard<std::mutex> lock(out.messMtx);
                // some output data
                out.serverMessages.push("some string 1");
                out.serverMessages.push("some string 2");
                // completed this iteration
                out.cv.notify_one();
            }
            catch (const std::exception& ex)
            {
                std::cerr << ex.what() << std::endl;
            }
            std::this_thread::sleep_for(std::chrono::seconds(1));
        }
    }

private:
    MessageQueue& out;
};

class InstrumentInterface {
public:
    virtual ~InstrumentInterface() = default;
};

class InstrumentalServer : public InstrumentInterface
{
public:
    InstrumentalServer() = default;

    void addDevice(AbstractDevice* dev) { device = dev; };

    void waitForMessages()
    {
        for (;;)
        {
            std::unique_lock<std::mutex> lock(messages.messMtx);
            // sleep until messages received
            messages.cv.wait(lock, [this]() {return !messages.serverMessages.empty(); });
            while (!messages.serverMessages.empty())
            {
                device->processData(messages.serverMessages.front());
                messages.serverMessages.pop();
            }
        }
    }

private:
    MessageQueue messages;
    std::unique_ptr<SocketServer> sockServer = std::make_unique<SocketServer>(messages);
    AbstractDevice* device;
};

int main()
{
    AbstractDevice device;
    InstrumentalServer iServer;
    iServer.addDevice(&device);
    iServer.waitForMessages();
    return 0;
}

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

Comments

[Андрей]

Предыдущая итерация этого кода, была почти как у вас. Я решил переделать все, поскольку мне ненравились спящие потоки, я ранее читал что потоки должны быть всегда заняты работой и оставлять их спящими плохая практика (возможно я не так это понял). В целом, как вы написали передавать в низкоуровневый класс this тоже не очень хорошая идея, у сервера своя задача по принятю и отправке данных, а не по выполнению дальнейших действий с устройством.

[menkar3]

Андрей, спящий поток - в принципе, ничего страшного не делает, насколько я знаю. Если так подумать, одновременно у нас в системе запущена куча приложений с кучей потоков в каждом, реально работают - максимум количество ядер. Можно было бы пофантазировать в сторону тредпула с переключением потока на другую задачу, если потоку сейчас делать нечего, но тогда нужно сохранять контекст/загружать его и т.д. Операционная система и так делает все это за нас:) В юзерспейсе спящий поток может и не вносить накладных расходов - в случае винды, например, зависнет на каком-нибудь вызове NtWaitForSingleObject, к примеру, дальше ос сама разберется когда его будить.

[menkar3]

А вообще - можно передать серверу коллбек в виде std::function, который он будет дергать при поступлении данных. Тогда можно будет передать ему лямбду c захватом this и эффективно вызывать метод у InstrumentalServer при этом ничего не зная о нем, кроме сигнатуры метода, к примеру

class SocketServer
{
public:
...
    void setCallback(std::function<void(std::string)>);
...
private:
    std::function<void(std::string)> callback;
};

И использовать как

InstrumentalServer::InstrumentalServer()
{
    server->setCallback([this](std::string s){ writeCommand(s); });
}

[Андрей]

menkar3, к сожалению функцию коллбэк находящиюся в классе сохранить нет возможности, ошибка доступа, когда я пытался сохранить ее в SocketServer. Пытался и лямбду с this кинуть.

[Андрей]

menkar3,

void setCallback(std::function<void(std::string)> coll){
callback = coll;//тут ошибка
}

[menkar3]

Андрей, только что быстро попробовал что-то такое, все ок

struct CbSource
{
    int i = 5;

    void callback()
    {
        std::cout << i << std::endl;
    }
};

struct CbDest
{
    void setCallback(std::function<void()> cb)
    {
        callback = std::move(cb);
    }

    void call()
    {
        callback();
    }

    std::function<void()> callback;
};

int main()
{
    CbSource src;
    CbDest dst;

    dst.setCallback([self=&src](){ self->callback(); });
    dst.call();
    return 0;
}

Ошибка доступа - может, проблемы с временем жизни? Или поток запустился, а коллбек еще не установлен?

[menkar3]

Андрей, Этот не пытался запускать, писал с телефона) Пример с dest и src точно рабочий

[Андрей]

menkar3, Знаешь, а он работает. Видимо в прошлый раз что то упустил, но со временем жизни точно проблем небыло. Кстати первый код это некоторый извращенный вариант коллбэка)