Code review - prosty serwer

https://github.com/Kbargi/simpleChatServer

Serwer pozwala na założenie pokoju czatowego i przyłączanie się do takiego pokoju rozmówców.
Generalnie sam pomysł z czatem był pierwszą rzeczą jaka mi wpadła do głowy i bardziej chodzi mi o uzyskanie krytyki ogólnej architektury tzn. co do podejścia do obsługi TCP, wątków za pomocą Listenera, Handlera i ThreadPool. Oczywiście uwagi co do całości też są dla mnie bardzo istotne.

W katalogu test_tools są skrypty pythona do podstawowych testów.
Pliki nagłówkowe są w libs/headers, a źródła są w libs/src

Z góry dziękuję za uwagi

1. Makefile
Review zacznę do Makefile-a, bo jest tam sporo (drobnych) błędów.
Nie żeby źródła programu mnie nie interesowały, po prostu review kodu w C++ jest trochę bardziej czasochłonne, więc zostawię sobie na deser :)
Generalnie rzecz biorąc ręczne pisanie Makefile-ów jest uciążliwe i błędogenne dlatego odchodzi się od tego na rzecz cmake-ów, ninje-ów, etc.
Zachęcam autora wątku żeby sam spróbował np. cmake-a.

1. Dobra praktyka to Makefile a nie makefile. Patrz [1].

2. Nie CPP=g++ oraz nie CPPFLAGS. CPP to predefiniowana zmienna preprocesora, CPPFLAGS to predefiniowana zmienna trzymająca flagi dla preprocesora. Zamienić odpowiednio na
   CXX oraz CXXFLAGS.
   <Offtop>
   Hardkodowanie w Makefile CXX jest sprawą dość śliską, bo utrudnia cross kompilację, gdyby CXX nie była ustawiana w środku Makefile to bez problemu zrobiłbym

export CXX=arm-linux-gnueabihf-g++; make -j 

i po 5s dostałbym serwer skompilowany na architekrurę arm (modulo zależności, aczkolwiek tutaj nie spodziewam się problemów bo twój projekt zależy tylko od boost-a, protobuf-a glibc-a - wszystko dostępne na armie). Teraz muszę ręcznie modyfikować CXX co jest kiepskie.

EDIT:
Jak zauważył @kq ostatnie zdanie NIE jest prawdziwe. Mimo zahardkodowania CXX nadal może zostać przekazane z zewnątrz za pomocą:

make CXX=arm-linux-gnueabihf-g++ 

Więcej info na ten tamat znaleźć można np. pod [2] oraz w dokumentacji make-a
</Offtop>

3. Linuksowa konwencja/tradycja jest taka, że binarki nie mają żadnych rozszerzeń. Dlatego zamiast server.out wystarczy po prostu server.

CPPFLAGS=--std=c++11 -s -Wall -lpthread -lboost_system -lboost_thread -pedantic -pthread -lboost_program_options 

Trochę nie rozumiem kombinacji flag. Z jednej strony -Wall może sugerować debug build, ale to gryzie się ze stripowaniem przez -s.
Być może miałeś dobre powody żeby stripować, ale z mojego doświadczenia -s jest rzadko przydatne. Stripowanie wycina wszystkie sekcje z binarki (zysk kilkunastu/kilkudziesięciu % rozmiaru binarki) przez co tracisz informacje o symbolach i debugowanie takiego programu jest b.trudne bo odbywa się na poziomie asemblera a nie C/C++.
Zwykle nieużywanie -g oraz włącznie max. optymalizacji znacznie mocniej redukuje rozmiar kodu wynikowego i jednocześnie zostawia furtkę dla gdb.

Moja sugestia jest taka, żeby debug build robić z flagami (+ upgrade do -std=c++14, o ile masz g++ >= 4.9, jeśli nie masz to zmień dystrybucję OS-a:)

CXXFLAGS=--std=c++14 -Wall -W -Werror -g

a release build przez coś w stylu:

CXXFLAGS=--std=c++14 -Wall -W -Werror -Ofast

Oczywiście jest to kwestia mocno dyskusyjna, dużo zależy od projektu, architektury sprzętowej, zasobów, etc.

5. Dobra praktyka to $(RM) nie rm. Patrz [1].

6. Nie no, serio nie podoba mi się ten Makefile, rozdmuchany i rozwlekły, ty masz tylko 7 jednostek kompilacji na krzyż, a co jeśli będzie ich 70? Ktoś mądry wymyślił sposób, żeby nie wypisywać tego ręcznie, robi się to za pomocą wildcard-ów. Robisz coś w stylu:

CPP_SRCS := $(wildcard $(SOURCES)*.cpp)
CPP_OBJS := ${CPP_SRCS:.cpp=.o}

server: $(CPP_OBJS)
	...

Więcej o tym tradycyjnie pod [1].

Ref:
[1] https://sites.google.com/site/michaelsafyan/software-engineering/how-to-write-a-makefile
[2] http://stackoverflow.com/questions/2826029/passing-additional-variables-from-command-line-to-make
C.D.N.

Wielkie dzięki za otrzymane uwagi, biorę się za poprawki i analizę materiału ;)

2. Valgrind/memcheck i zwalnianie zasobów
Review ciąg dalszy. Tym razem zobaczymy jak twoja apka zarządza stertą, file descryptorami, czy coś przez przypadek nie cieknie. Zaczynając od polowania na memory leak-i, uruchamiając apkę:

valgrind --tool=memcheck --leak-check=yes --show-reachable=yes --num-callers=20 --track-fds=yes ./server.out --config conf/config.conf &> valgrind.txt

I ubijając ją po kilknastu sekundach przez Ctrl+C dostajmy 200KB dump z 34 błędami :(
Podsumowanie memcheck-a:

==3740== Memcheck, a memory error detector
==3740== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==3740== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==3740== Command: ./server.out --config conf/config.conf
==3740==
THREADS: 5
Handler 3 started
Handler 7 started
Handler 5 started
==3740==
==3740== FILE DESCRIPTORS: 10 open at exit.

==3740== LEAK SUMMARY:
==3740== definitely lost: 0 bytes in 0 blocks
==3740== indirectly lost: 0 bytes in 0 blocks
==3740== possibly lost: 7,186 bytes in 61 blocks
==3740== still reachable: 9,736 bytes in 146 blocks
==3740== suppressed: 0 bytes in 0 blocks
==3740==
==3740== For counts of detected and suppressed errors, rerun with: -v
==3740== ERROR SUMMARY: 34 errors from 34 contexts (suppressed: 0 from 0)

Wniosek 1: Apka nie radzi sobie z obsługą Ctrl+C - nie ma handlera obsługi sygnału SIGINT, przez co nie są wołane destruktory obiektów,
nie są zamykane deskryptory i w efekcie apka cieknie. IMO warto coś takiego doimplementować bo effort niewielki (kilka linijek z sigaction + volatile) a
profesjonalne serwery takie jak np. nginx i h2o mają taki mechanizm "gratuitous shutdown-u" wbudowany z miejsca.

Uznałem, że trzeba dać apce jeszcze jedną szansę. Zmodyfikowałem delikatnie kod tzn dodałem m_stop = true; w pętli pod Listener::run tak, żeby pętla obróciła się jeden raz i po 10s zakończyła (kiedy strzeli timeout select-a):

 
  while (!m_stop) {
    read_fds = m_master;
    if (select(m_fdmax + 1, &read_fds, NULL, NULL, &tv) == -1)
      throw std::runtime_error(std::string("Main select failed"));
    // run through the existing connections looking for data to read
    for (int i = 0; i <= m_fdmax; i++) {
      if (FD_ISSET(i, &read_fds)) {  // we got one!!
        if (i == m_listener) {
          this->handleNewConnection();
        } else {
          std::cout << "unknown socket " << i << " writes to main select\n";
        }
      }  // END got new incoming connection
    }  // for through existing connections
    m_stop = true; //<------------------------- dodana linijka
  }  // main while

Apka nie zawisła jak ostatnio ale sama ładnie zamknęła się po 10s.
Flow wszedł do destruktorów, więc wszystko powinno być OK. No ale nie było. Dump skurczył się do ~70KB. Leaki pozostały :(
Podsumowanie memcheck-a:

==3795==
==3795== FILE DESCRIPTORS: 4 open at exit.
==3795== LEAK SUMMARY:
==3795== definitely lost: 0 bytes in 0 blocks
==3795== indirectly lost: 0 bytes in 0 blocks
==3795== possibly lost: 1,485 bytes in 22 blocks
==3795== still reachable: 5,216 bytes in 75 blocks
==3795== suppressed: 0 bytes in 0 blocks
==3795==
==3795== For counts of detected and suppressed errors, rerun with: -v
==3795== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

Wniosek 2: Apka cieknie nawet jeśli jest zamykana po "Bożemu". Czemu? Praktycznie cały dump składa się z call stack-ów lecących z wnętrza protobuf-a.
Przykładowy dump:

== 56 bytes in 1 blocks are still reachable in loss record 31 of 64
==3795== at 0x4C2C100: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3795== by 0x557E5AB: std::_Rb_tree<std::string, std::pair<std::string const, std::pair<void const*, int> >, std::_Select1st<std::pair<std::string const, std::pair<void const*, int> > >, std::lessstd::string, std::allocator<std::pair<std::string const, std::pair<void const*, int> > > >::M_insert(std::_Rb_tree_node_base*, std::_Rb_tree_node_base*, std::pair<std::string const, std::pair<void const*, int> > const&) (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.1)
==3795== by 0x557EBAC: google::DescriptorIndex<std::pair<void const*, int> >::AddSymbol(std::string const&, std::pair<void const*, int>) (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.1)
==3795== by 0x5580C7C: google::DescriptorIndex<std::pair<void const*, int> >::AddFile(google::FileDescriptorProto const&, std::pair<void const*, int>) (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.1)
==3795== by 0x557C2BD: google::Add(void const*, int) (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.1)
==3795== by 0x5536DD9: google::InternalAddGeneratedFile(void const*, int) (in /usr/lib/x86_64-linux-gnu/libprotobuf.so.9.0.1)
==3795== by 0x145641: chat::protobuf_AddDesc_chat_2eproto() (in /home/yurai/simpleChatServer/server.out)
==3795== by 0x14B89B: chat::StaticDescriptorInitializer_chat_2eproto() (in /home/yurai/simpleChatServer/server.out)
==3795== by 0x149FA1: __static_initialization_and_destruction_0(int, int) (in /home/yurai/simpleChatServer/server.out)
==3795== by 0x149FDD: _GLOBAL__sub_I__ZN4chat32protobuf_AssignDesc_chat_2eprotoEv (in /home/yurai/simpleChatServer/server.out)
==3795== by 0x15615C: __libc_csu_init (in /home/yurai/simpleChatServer/server.out)
==3795== by 0x5F5D9CE: (below main) (libc-start.c:245)

Okazuje się, że przyczyna to brak wywołania ShutdownProtobufLibrary() na końcu twojego programu. Bez tego wywołania protobuf nie sprząta po sobie, a
memcheck raportuje błędy z wnętrza biblioteki. Źródło:
https://groups.google.com/forum/#!topic/protobuf/HB1c7KN2AM4

Po wprowadzeniu poprawki dump zredukował się do ~1.5KB. Pozostał jedynie problem z socket-em m_listener, który jest tworzony w Listener:

 
if ((m_listener = socket(p->ai_family, p->ai_socktype, p->ai_protocol)) <0) {

ale nie jest zamykany w Listener::stop. Ostatecznie po naprawieniu tego błędu i dodaniu do Listener::stop linijki:

 close(m_listener); 

raport z memchecka:

==3908== HEAP SUMMARY:
==3908== in use at exit: 0 bytes in 0 blocks
==3908== total heap usage: 993 allocs, 993 frees, 60,459 bytes allocated
==3908==
==3908== All heap blocks were freed -- no leaks are possible

Memory leaki zniknęły. C.D.N.

3. Implementacja/C++

Tym razem findingi znalezione w źródłach apki. Będzie ich sporo, ale znowu podobnie jak w Makefile-u nie są to jakieś grube problemy.

1. Using zamiast typedef

2. Te same nazwy dla pól i metod z różnych klas. Jest Listener::m_stop i Handler::m_stop co może wprowadzać w błąd. Podobnie z Handler::handleNewConnection
   oraz Listener::handleNewConnection.

3. Od c++14 make_unique zamiast new

4. Lock_guard zamiast unique_lock. Dobrą praktyką jest stosowanie lock_guarda wszędzie gdzie to tylko możliwe.
   Lock_guard działa na zasadzie RAII, cały jego interfejs to konstruktor i destruktor. Konstruktor lock-uje, a destruktor unlock-uje. Z kolei Unique_lock ma dość mocno rozbudowany interfejs (wystawia m.in. lock(), unlock(), try_lock() etc.), który przydaje się w zasadzie tylko do wspołpracy z condition_variable. Czyli unique_lock tylko dla ThreadPool, w pozostałych przypadkach użyj lock_guarda.
   Ref: http://stackoverflow.com/questions/20516773/stdunique-lockstdmutex-or-stdlock-guardstdmutex

5. Jak już jesteśmy przy lock-ach i multithreadingu to warto użyć helgrinda do polowania na race conditions:

valgrind --tool=helgrind ./server.out --config conf/config.conf &> helgrind.txt

W przypadku twojej apki helgrind raportuje trochę problemów (większość to false positivy więc nie ma sensu o nich wspominać). Wspomnę tylko o jednym realnym problemie. Helgrind raportuje:

"Possible data race during read of size 8"

dla Handler::operator()() (Listener.cpp:4) czyli dla

  std::cout << "Handler " << m_readPipeSocket << " started\n";.

Z opisu wynika, że thread #8 czyta a thread #7 pisze wewnątrz operatora<< w std::cout. Std::cout sam w sobie jest thread-safe (nie ma data race-ów przy równoczesnych read-ach/writeach). Może jednak wystąpić race condition polegający na tym, że tekst na konsoli może być poprzeplatany (np. napisy "Handler 3", "Handler 5" skleją się do "HandlerHandler 3 5"). Lepiej dla pewności ochronić lock_guardem.
Ref: http://stackoverflow.com/questions/6374264/is-cout-synchronized-thread-safe?lq=1

 Std::this_thread::sleep_for(std::chrono::seconds(4)) 

w ThreadPool.cpp; Nie jestem pewny po co jest ten sleep, ale sleep (w tak wysokich warstwach oprogramowania i NIE w testach) prawie zawsze oznacza jakiś problem synchronizacji, który powinien zostać rozwiązany za pomocą semaforów/condition_variables.

7. Mieszasz C z C++. Słówko struct jest w wielu miejscach redundantne. Jest:

 struct timeval, struct sockaddr_storage, struct sockaddr*, struct sockaddr_in*, struct sockaddr_in6*

a wystarczy krótko:

timeval, sockaddr_storage,  sockaddr* 

itd.
8. Znowu. W C++ jest constexpr zamiast #define dla stałych liczbowych
9. Mieszasz C z C++ nawet bardziej. W C++ jest std::vector/std::array zamiast plain c-array dla tablic

10. Config(std::string mainConf) stringi przekazuj przez const referencję - Config(const std::string &mainConf)

11. Metody Config::dump i Config::get mogą być constowe.

12. Z kolei const przy typie zwracanym przez wartość nie ma sensu (zresztą -Wextra o tym przypomina). Zatem żadna z metod User::getSocket, SharedQueue::size i PrioritySharedQueue::size nie powinna zwracać typu const size_t ale po prostu size_t.

13. Range based for w ThreadPool::stop zamiast explicit for

14. *.hpp zamiast *.h

15. Klasa Listener ma wirtualne init, run i destruktor, ale żadna klasa nie overriduje tych metod (właściwie to żadna klasa nie dziedziczy po Listener-rze). Co więcej Listener nie jest też nigdzie wykorzystywany w polimorficzny sposób, tzn. w main-ie tworzysz sobie Listenera na stosie, więc nie ma szans na dynamiczne wiązanie. Nie jestem pewny czy Listener jest w ogóle dobrym kandydatem na posiadanie metod wirtualnych.

16. Listener-a i Handler-a wydzieliłbym do oddzielnych plików, podobnie z RequestHandlerTask oraz OnUserEscapeTask aczkolwiek to jest kwestia gustu

17. Z tego co rozumiem jedynym sensem życia klas: AbstractTask, RequestHandlerTask oraz OnUserEscapeTask jest wołanie w nieco pokrętny sposób (przez virtual operator())

 m_roomManager->processRequest(m_clientSocket, m_request); 

oraz

 m_roomManager->processEscape(m_clientSocket);

prawda?
Zauważ, że naprawdę nie potrzeba tutaj całej grupy klas *Task związanych ze sobą hierarchią. Te klasy to tak naprawdę funktory, trzymające jakiś tam kontekst w stylu m_roomManager, m_clientSocket oraz opcjonalnie m_request. W C++14 jest już coś takiego zaimplementowane. Nazywa się lambda oraz std::function :)
Zatem zamiast robić:

 
m_pool->add(
        std::make_shared<OnUserEscapeTask>(clientSocket, m_roomManager));
m_pool->add(std::make_shared<RequestHandlerTask>(
          clientSocket, m_roomManager, std::move(request)));

wystarczy:

• zmienić syganturę ThreadPool::add(PTask task) na ThreadPool::add(std::function<void(void)> task) oraz wszędzie PTask na std::function<void(void)>.
• wołać

 
m_pool->add([m_roomManager, m_clientSocket](){
	m_roomManager->processEscape(m_clientSocket);
});
m_pool->add([m_roomManager, m_clientSocket, m_request](){
	 m_roomManager->processRequest(m_clientSocket, m_request);
});

Lamda jest automagicznie konwertowana na std::function<void(void)>, wszystko działa i klasy *Task można wywalić :)

Tak na marginesie dwa problemy, które napotkałem przy uruchamianiu twojego programu z użyciem valgrinda i gdb:

1. Strippowanie nie tylko utrudnia debuggowanie (w kontekście poprzedniego posta), ale też analizę valgrindem bo dostajemy callstacki bez nazw symboli (zamiast tego są offsety). Kolejny powód, żeby zrezygnować z flagi -s.

2. Przez flagę -fPIE dla g++ (Position independent code) gdb nie chciał zatrzymywać się na breakpointach. Okazało się, że jest to bug gdb (serio, nie żartuje)
   (http://stackoverflow.com/questions/23553527/gdb-error-in-re-settings-breakpoint-cannot-access-memory), po usunięciu -fPIE z CXXFLAGS breakpointy zaczęły działać:)

Mimo tylu findingów IMO kod apki jest naprawdę na całkiem przyzwoitym poziomie, w sumie dawno nie widziałem na tym forum tak zaawansowanego projektu w C++ (o otwartych źródłach oczywiście). W kolejnym odcinku opiszę problemy designowe twojej aplikacji oraz sposoby na ich rozwiązanie (łącznie z dobrymi wzorcami stosowanymi w tego typu projektach).
C.D.N.

1. Zmieniłem czystego make na cmake

Mam pytanie odnośnie Twojej propozycji zmiany z klasami Task:

 m_pool->add([m_roomManager, m_clientSocket](){
    m_roomManager->processEscape(m_clientSocket);
});
m_pool->add([m_roomManager, m_clientSocket, m_request](){
     m_roomManager->processRequest(m_clientSocket, m_request);
});

Czy potrafisz wskazać jakiś dogodny sposób by przy Twoim rozwiązaniu te lambdy mogły być priorytetyzowane jak to ma miejsce w przypadku rozwiązania z klasami Task* ?

 
class TaskComparator {
 public:
  bool operator()(const std::shared_ptr<AbstractTask> t1,
                  const std::shared_ptr<AbstractTask> t2) {
    return t1->getPriority() > t2->getPriority();
  }
};

1. Zmieniłem czystego make na cmake

Dobrze.

2. Czy potrafisz wskazać jakiś dogodny sposób by przy Twoim rozwiązaniu te lambdy mogły być priorytetyzowane jak to ma miejsce w przypadku rozwiązania z klasami Task* ?

Szczerze mówiąc nie zauważyłem, że pole m_priority jest używane w ten sposób przez TaskComparator. Std::function nie pozwala wg mojej wiedzy na dostęp do kontekstu (w tym wypadku na dostęp do m_priority) z zewnątrz bez hakowania/"reinterpret_casto-wania" prywatnych bebechów. W takim razie nie zrezygnujemy całkowicie z klasy AbstractTask, jednak mimo to nadal możemy wyrzucić RequestHandlerTask oraz OnUserEscapeTask.

Proponuję przerobić:

 
class AbstractTask {
 public:
  AbstractTask(TaskPriority prior) : m_priority(prior) {}
  virtual ~AbstractTask(){};
  virtual void operator()() = 0;
  virtual void setPriority(TaskPriority p) { m_priority = p; }
  virtual TaskPriority getPriority() { return m_priority; }

 protected:
  TaskPriority m_priority;
};

na coś na kształt:

class Task {
 public:
  AbstractTask(TaskPriority prior) : m_priority(prior) {}
  void setPriority(TaskPriority p) { m_priority = p; }
  TaskPriority getPriority() { return m_priority; }
  void addTask(const std::function<void(void)> &_task);

 protected:
  TaskPriority m_priority;
  std::function<void(void)> task;
};

Use case klasy Task wyglądałby wtedy np:

Task escapeTask(NORMAL);
escapeTask.addTask([m_roomManager, m_clientSocket](){
    m_roomManager->processEscape(m_clientSocket);
});
m_pool->add(escapeTask);

Task requestTask(HIGH);
requestTask.addTask([m_roomManager, m_clientSocket, m_request]((){
    m_roomManager->processRequest(m_clientSocket, m_request);
});
m_pool->add(requestTask);

Oczywiście addTask jest luźną propozycją, handler może być równie dobrze przekazywany w konstruktorze w duchu RAII. Poza tym std::function lubi być forwardowane przez r-value reference, więc to też możesz poprawić. To są jednak detale. Tak czy siak zgodnie z tym co napisałem ostatnio w addTask następuje magiczna konwersja lambdy na std::function i Jeśli niczego nie przeoczyłem/pokręciłem, to RequestHandlerTask i OnUserEscapeTask są w tym momencie do wywalenia :)