Ваш Init выглядит хорошо:
- создать сокет, связать его, прослушать его, начать принимать поток
В вашей принимающей ветке acceptClient выглядит нормально:
- распечатать сообщение
- добавить сокет клиента в
clients очередь
- создать новую принимающую тему
Ваш Run не имеет смысла:
- создать один поток на элемент в
clients для получения от прослушивания socket
Похоже, что вы создаете новый поток для каждого отдельного действия сокета. Это довольно расточительный дизайн. Как только поток завершен, он может снова заняться чем-то другим.
Таким образом, создание нового потока принятия в acceptClient - пустая трата времени, вы можете просто вернуться к началу ::accept следующего клиента. Вот так:
acceptClient() {
while (alive) {
int client = accept(socket, ...);
createClientHandler(client);
}
}
То, что, по-видимому, отсутствует, порождает новый клиентский поток для обслуживания клиентского сокета. В настоящее время вы делаете это в Run, но это до того, как любой из клиентов будет принят. И вы делаете это для неправильного сокета! Вместо этого вы должны порождать потоки receiveFromSocket в acceptClient, и , передавая им client socket. Так что это ошибка.
В вашем receiveFromSocket вам также не нужно создавать еще один поток для receiveFromSocket снова - просто вернитесь к началу.
Наибольшее беспокойство в связи с этим проектом «поток за действие» заключается в том, что вы создаете потоки отправителей для каждого входящего сообщения. Это означает, что у вас может быть несколько потоков отправителей, пытающихся набрать ::send в одном и том же сокете TCP. Это не очень безопасно.
Порядок вызовов, выполняемых в WSASend, также является порядком, в котором буферы передаются на транспортный уровень. WSASend не должен вызываться на одном и том же потоково-ориентированном сокете одновременно из разных потоков, поскольку некоторые провайдеры Winsock могут разделить большой запрос на отправку на несколько передач, и это может привести к непреднамеренному чередованию данных из нескольких одновременных запросов на отправку в одном и том же ориентированном на поток розетка.
https://docs.microsoft.com/en-us/windows/desktop/api/winsock2/nf-winsock2-wsasend
Аналогично, вместо того, чтобы создавать потоки в broadcastToClients, я предлагаю вам создать один постоянный поток отправителя на каждый сокет клиента в acceptClient (вместе с потоком receiveFromSocket внутри некоторого createClientHandler).
Для связи с потоками отправителя вы должны использовать очереди блокировки потоков. Каждый поток отправителя будет выглядеть так:
while (alive) {
msg = queue.next_message();
send(client_socket, msg);
}
Затем в полученном сообщении вы просто делаете:
for (client : clients) {
client.queue.put_message(msg);
}
Итак, подведем итог: для обработки каждого клиента вам нужна такая структура:
struct Client {
int client_socket;
BlockingQueue queue;
// optionally if you want to keep track of your threads
// to be able to safely clean up
std::thread recv_thread, send_thread;
};
Безопасная очистка - это совсем другая история.
Наконец, комментарий к этому комментарию в вашем коде:
// Wait for all threads to finish
for(std::thread& t : threads)
{
t.detach();
}
Это почти противоположно тому, что std::thread::detach делает:
https://en.cppreference.com/w/cpp/thread/thread/detach
Это позволяет вам уничтожить объект потока, не дожидаясь окончания выполнения потока.