Wątki i mutex-y

Witam

Chciałem napisać prosty demon TCP w C++, który służyłby mi jako prosty chat. W tym celu chciałem skorzystać z wielowątkowości biblioteki standardowej, lecz już w prostym przypadku natrafiam na problemy.

Moja koncepcja jest taka:
UserDispatcher : std::thread - klasa która będzie zarządzać połączeniami od nowych użytkowników.
User : std::thread - klasa obsługująca danego użytkownika

Na tę chwilę, celem uproszczenia, w wątku dispatchera nowy użytkownik tworzony jest co 2s, i każdy z nich wyświetla na standardowe wyjście w swoim imieniu swoje id. Poza tym nie dzieje się nic.
Teraz w przypadku gdy w funkcji run() dispatchera nowego użytkownika stworzę jawnie wołając "users.push_back(User(&mutex));" to program działa zgodnie z moim oczekiwaniem, natomiast zamknięcie tego fragmentu kodu w funkcję addUser() powoduje iż na standardowe wyjście zgłaszają się użytkownicy z id=0. Cóż robię nie tak?

Tzn. output w pierwszym przypadku:

New user
User thread entered, id: 0
New user
User thread entered, id: 1
New user
User thread entered, id: 2

W przypadku drugim:

New user
User thread entered, id: 0
New user
User thread entered, id: 0
New user
User thread entered, id: 0

Klasa UserDispatcher:

class UserDispatcher: public std::thread {
    std::mutex mutex;
    std::list<User> users;

    void run(void);
    void addUser(void);

public:
    UserDispatcher(void);
};

void UserDispatcher::run(void) {
    while(true) {
        mutex.lock();
        std::cout << "New user" << std::endl;
//        users.push_back(User(&mutex));     //pierwszy przypadek
//        addUser();                        //drugi przypadek
        mutex.unlock();
        std::this_thread::sleep_for(std::chrono::seconds(2));
    }
}

void UserDispatcher::addUser(void) {
    users.push_back(User(&mutex));
}

UserDispatcher::UserDispatcher(void)
        : std::thread(&UserDispatcher::run, this) {
}

Klasa User:

class User: public std::thread {
    static int nextID;
    std::mutex *mutex;
    int id;

    void run(void);

public:
    User(std::mutex *mutex);
};

void User::run(void) {
    mutex->lock();
    std::cout << "User thread entered, id: " << id << std::endl;
    mutex->unlock();
    while(true) {
        std::this_thread::sleep_for(std::chrono::milliseconds(100));
    }
}

User::User(std::mutex *mutex)
        : mutex(mutex), id(nextID++), std::thread(&User::run, this) {
}

oba przypadki są złe, zauważ że robisz dual lock na tym samym mutexie, a potem zwalniasz go w User pozwalająć wątkowi, w którym jest dispatcher, jechać dalej (to jest jeden z use casów)

masz zły design

Robię tak, bo tak zrozumiałem działanie mutexów. Robię lock w UserDispatcher, po to, by nowo tworzony wątek w User również zrobił lock na tym mutexie, co powinno spowodować jego zatrzymanie w tym miejscu, do momentu, gdy UserDispatcher wykona unlock na mutexie.

Nie sądzę żeby projektanci std::thread projektowali tą klasę by po niej dziedziczyć.

omicronns napisał(a):

Robię tak, bo tak zrozumiałem działanie mutexów. Robię lock w UserDispatcher, po to, by nowo tworzony wątek w User również zrobił lock na tym mutexie, co powinno spowodować jego zatrzymanie w tym miejscu, do momentu, gdy UserDispatcher wykona unlock na mutexie.

true ale chodzi o to, że sharujesz mutex między 2 totalnie rózne obiekty, albo zrób osobnego mutexa dla Userów, albo nadawaj im ID w dispatcherze i bez lockowania i unlockowania w userze

Przede wszystkim @omicronns +1 za ciekawy, techniczny wątek, o które na tym forum bardzo ciężko ;)

Teraz to meritum.
Na początek trzeba cię skarcić za dziedziczenie po std::thread (wspomniał już o tym @several). Nie chodzi tutaj tylko o to, że std::thread nie ma wirtualnego destruktora (więc w przypadku trzymania obiektów zdefiniowanych przez ciebie klas w referencji/wskaźniku otrzymasz wyciek pamięci). Główny problem to kolejność inicjalizacji na liście inicjalizacyjnej klasy. W tym miejscu odsyłam do dokumentacji/standardu. W każdym razie w twoim przypadku najpierw utworzy się funkcja wątku (nowy wątek), kolejno nastąpi skopiowanie wskaźnika na std::mutex, a na końcu dojdzie do postrinkrementacji statycznej zmiennej i skopiowania jej wartości do pola id. Dodam, że to wszystko mógłbyś wykryć dodając flagę kompilacji -Wreorder. Ergo, twój wątek będzie działał na niezainicjowanych zmiennych. Co więcej użycie this na liście inicjalizacyjnej w przypadku, gdy pola danej klasy nie są jeszcze zainicjowane to UB. Tyle z teorii, rzućmy okiem na działanie tego kodu w praktyce (pamiętając o tym, że to UB).

Jeśli używamy bezpośrednio users.push_back(User(&mutex)) to nie wiedzieć dlaczego ten kod działa xd. A po przeniesieniu tej instrukcji do osobnej metody dostajemy segfault. Co jest bardzo dziwne, bo w obu przypadkach używany jest ten sam, defaultowy move constructor. Jedyna różnica jest taka, że w tym drugim przypadku mutex nie jest jeszcze zainicjowany i przez to dostajemy segfault. Sprawdzałem to na g++ 4.9.2 i clang++ 3.5.0 i w obu przypadkach zachowanie było jak to opisane powyżej. Tak, tak wiem, że to UB więc równie dobrze mógłbym się spodziewać tego, że spali mi się karta graficzna, komputer odegra Hejnał Maryjacki, etc. Mimo wszystko ciekawe zagadnienie ;)

Jeszcze jedna sprawa. @omicronns z tego co widzę chcesz zapewnić atomość dostępu do io (w tym wypadku tylko stdout). Polecam w tym celu użycie rozwiązanie zaproponowanego przez Howarda Hinnant, które poza tym, że działa to jeszcze zachowuje się zgodnie z zasadą pojedynczej odpowiedzialności bytów.

#include <iostream>
#include <mutex>

std::ostream&
print_one(std::ostream& os)
{
    return os;
}

template <class A0, class ...Args>
std::ostream&
print_one(std::ostream& os, const A0& a0, const Args& ...args)
{
    os << a0;
    return print_one(os, args...);
}

template <class ...Args>
std::ostream&
print(std::ostream& os, const Args& ...args)
{
    return print_one(os, args...);
}

template <class ...Args>
std::ostream&
print(const Args& ...args)
{
    static std::mutex m;
    std::lock_guard<std::mutex> _(m);
    return print(std::cout, args...);
}

// Użycie

void exec()
{
    print("Hello ", std::this_thread::get_id(), '\n');
    std::this_thread::sleep_for(std::chrono::milliseconds(100));
}

Edit: @omicronns zastanów się jeszcze nad samym rozwiązaniem problemu. Tip, wątek per klient to nie jest dobre rozwiązanie.