[Review] Serveur TCP/IP (Testé avec 10,000 clients)

Bonjour !

Ce serveur est le premier d'une série de 3 qui suivront bientot, vous avez ici la structure du point d'entrée du serveur de mon jeu.

C'est également la structure de base des 3 autres.

Pour voir les 10,000: http://rpg.neorath.nl/viewtopic.php?...174a88fcd6e#p8

Bref,

Je suis parti d'un niveau en C++ proche du vide interstellaire donc soyez dur :aie:
J'aimerais avoir vos commentaires sur la source, voir si je respecte bien la liste trop longue des concepts que je vois sur le salon des devs apps du chat!
De plus, le but de cette review est évidemment de confronter ce code à des pro, pour que vous m'aidiez à améliorer le serveur, car meme si il tourne à 10,000 clients, il reste un peu lent, j'aimerais dans l'ideal le faire tourner à 100,000 clients sans qu'il brule l'ordinateur ou il sera lancé ;)

Un grand merci à l'avance à:
Bousk, gbdivers, LittleWhite, fearyourself, Sehnsucht, Winjerome qui sont là depuis le début !

Bonne lecture, je reste pas loin pour les questions :)

nico

Bonjour,

Je ne comprend pas le contenu du fichier Errors.h . Il n'y a qu'une fonction "libre", pas de classe, pas de namespace.
Pourquoi les données ne sont pas chargés directement par ErrorMessage ? ou quelque chose de similaire (une fonction static/ une map par défaut, un template qui définit la map ?)
En plus, votre map à l'intérieure est static (je ne vois pas pourquoi d'ailleurs).

Sinon, je pensais qu'en C++, on aurait plus besoin des variadic arguments (les trucs comme dans printf), mais faut croire que si. (Je me demande pourquoi vous les utilisez en fait). Un template n'irait pas ?

Je suis triste de ne pas avoir de Makefile :cry:

Dans Database.h je trouve qu'il y a beaucoup trop de variable écrit en dur. Pourquoi ne pas faire un fichier de configuration ? (ou similaire). Qui serait chargé dans l'application, dynamiquement.

Le choix d'utiliser des #ifdef pour savoir si c'est le serveur maitre ou pas est, d'après moi, une grossière erreur. Il faudrait mieux faire une classe spécifique au client et une autre spécifique pour le maitre.

Code:

---

1 2	if(order == 0x01) // CMD_GOONLINE {

---

Au minimum, il faut utiliser un enum, au maximum, un design pattern :P

Code:

---

ServerPacketHandler(Session* t, Selector*& st, const std::string& linkname):

---

Déjà qu'il reste des pointeurs nus ... mais en plus, le Selector*& ... me fait froid dans le dos.

Sinon, je trouve qu'il y a trop de truc en static ... mais bon.

Une classe qui est hérité, doit avoir son destructeur en virtuel.

Code:

---

void ServerPacketHandler::handleRedirect(PacketReader& packet)

---

Possible que cela doit être const PacketReader& mais cela va vous embetter un peu :D

Le fichier ServeurSocket.h a un nombre d'include abérrant (surement trop)
Regardez aussi du coté de la prédéclaration (surement dans la FAQ C++)
(Meme remarque pour le Networker.h)

Pourquoi les fichiers de Session sont dans les Tools ?

Voilà pour le moment. Il serait génial de générer des diagrammes (Diagramme de classes et de dépendances) pour avoir un plus grand aperçu de l'architecture

Quelques remarques en vrac, en première relecture

* pareil que LittleWhite, j'aurais aimé avoir quelques chose pour faciliter la compilation (makefile)
* utilisation de C++11 (un peu ;) ) et exceptions -> très bien

dans main.cpp
* argc et argv non utilisé
* variables "Sis" et "Sls" : tu mets une majuscule habituellement à tes noms de classes et une minuscule à tes noms de variables, sauf là
* tu créés reg dans dès le début, alors que ne l'utilise pas forcement
* tu fais 2 fois la même séquence de code (query + création vecteur + storein), tu devrais factoriser ça peut être
* #include <string> inutile
* #include DatabaseStocks utile ? a priori, c'est pour créer les types ServerLinksStock et ServersLinksStock (d'ailleurs, les noms sont trop proches et peu lisibles = risque de confusion)
* LE PLUS IMPORTANT : je vois des new mais pas des delete -> fuites mémoire. Utilise unique_ptr<> ici (et vire tous les pointeurs nus dans tout le projet a priori)

dans Database.h
* tous les include sont nécessaire ?
* toutes les static const : à terme, à mettre dans un fichier de configuration
* c'est quoi BLOB_TRUE et BLOB_FALSE ?
* Database avec sémantique de singletion utile ? utilisé uniquement dans Session.cpp. Voir les nombreuses discussion là dessus sur le forum et sur le blog d'Emmanuel Deloget
* utilité d'avoir la classe en singletion et toutes les fonctions en static ?
* homogénéité des noms de variables (définir les conventions de codage et les respecter :) )
* delete instance -> utiliser unique_ptr<>
* Format(const std::string& sFormat) -> bizarre comme fonction, elle ne semble pas faire un format mais vérifier la présence d'un placeholder dans la chaine
* de plus, pour cette fonction, tu aurais pu faire Format(const std::string& sFormat, std::string s_Placeholder = "%") pour plus de souplesse dans le code
* manque de const aussi dans les fonctions Format
* la seconde fonction Format -> un variadic template 8O on en voit peu. Tu es familier avec le perfect forwarding ou tu as copié collé du code ? :D L'utilisation à l'air correcte. Par contre, je suis pas sur que cela doit être de la responsabilité d'une classe Database de faire cela, tu es sur de respecter le principe de responsabilité unique ? (SRP)
* fonction Exec : tu mélanges plusieurs écriture différentes. Soit ta classe est un singleton et tu dois écrire (comme tu l'as fait) :

Code:

---

1 2	Database* db = new Database(); mysqlpp::Query query = db->Exec(Database::Format("SELECT * FROM `servers` WHERE linkname ='%'", MyServerName));

---

et dans ce cas, Exec() n'a pas de raison d'être static et d'appeler Database::getInstance() en interne.
Soit on doit écrire :

Code:

---

mysqlpp::Query query = Database::Exec(Database::Format("SELECT * FROM `servers` WHERE linkname ='%'", MyServerName));

---

et dans ce cas, oui, Exec() doit être static, mais getInstance() et le constructeur n'ont pas de raison d'être public.
Là, tu autorises les 2 écritures, c'est bof (surtout que tu mets pas ensuite Store() en static donc que la première écriture, d'où risque de confusion pour l'utilisateur, qui peut utiliser 2 écritures pour certaines fonction et 1 seule pour d'autres)
* fonction Link() -> c'est un getter, utilité ? risque de non respect du principe d'encapsulation et exposition de détail interne ?

dans Database.cpp
* Database* Database::instance; -> variable non initialisée
* Database() -> ; en trop après la définition du corps du constructeur

Finalement, ta classe Database n'a pas d'autre fonction que de faire une query. Ca ne serait pas mieux de créer une classe Query et que ta classe Database (ou les 2/3 fonctions de Database) soit interne à Query (détail d'implémentation). Ca serait mieux en terme de sémantique

La suite plus tard...

Citation:

---

Envoyé par LittleWhite

Je ne comprend pas le contenu du fichier Errors.h . Il n'y a qu'une fonction "libre", pas de classe, pas de namespace.
Pourquoi les données ne sont pas chargés directement par ErrorMessage ? ou quelque chose de similaire (une fonction static/ une map par défaut, un template qui définit la map ?)
En plus, votre map à l'intérieure est static (je ne vois pas pourquoi d'ailleurs).

---

> Oui c'est en effet très moche ... Il va falloir que je change ca ...
Je devrais avoir plutot des #define blabla 1, et acceder au tableau via montableau[blabla] pour que mon code soit plus lisible plus tard... !

Citation:

---

Envoyé par LittleWhite

Sinon, je pensais qu'en C++, on aurait plus besoin des variadic arguments (les trucs comme dans printf), mais faut croire que si. (Je me demande pourquoi vous les utilisez en fait). Un template n'irait pas ?

---

Alors la je sais pas... Le variadic etait la solution la plus simple pour moi..!

Citation:

---

Envoyé par LittleWhite

Je suis triste de ne pas avoir de Makefile :cry:

---

Faut que jcomprenne comment faire ca :mouarf:

Citation:

---

Envoyé par LittleWhite

Dans Database.h je trouve qu'il y a beaucoup trop de variable écrit en dur. Pourquoi ne pas faire un fichier de configuration ? (ou similaire). Qui serait chargé dans l'application, dynamiquement.

---

Chargé dynamiquement.... ?

Citation:

---

Envoyé par LittleWhite

Le choix d'utiliser des #ifdef pour savoir si c'est le serveur maitre ou pas est, d'après moi, une grossière erreur. Il faudrait mieux faire une classe spécifique au client et une autre spécifique pour le maitre.

---

Hum, le #define est au niveau du projet, le zip contient les fichiers partagés par chaque serveur (le maitre ne possede que ces fichiers; le login en ajoute 2-3; le world encore plus) C'est la structure de chaque serveur et il a y quelques modifications en fonction justement dans quel projet le code se situe, donc le #ifdef me parait tres approprié!

Citation:

---

Envoyé par LittleWhite

Code:

---

1 2	if(order == 0x01) // CMD_GOONLINE {

---

Au minimum, il faut utiliser un enum, au maximum, un design pattern :P

---

Oui je sais :calim2: (noté dans mon projet)

Citation:

---

Envoyé par LittleWhite

Code:

---

ServerPacketHandler(Session* t, Selector*& st, const std::string& linkname):

---

Déjà qu'il reste des pointeurs nus ... mais en plus, le Selector*& ... me fait froid dans le dos.

---

Haha, je sais meme plus pourquoi j'avais mis le & ... Mais mes pointeurs sont bien en tout cas!

Citation:

---

Envoyé par LittleWhite

Sinon, je trouve qu'il y a trop de truc en static ... mais bon.

---

Ha bon ? C'est grave?

Citation:

---

Envoyé par LittleWhite

Une classe qui est hérité, doit avoir son destructeur en virtuel.

---

noté !

Citation:

---

Envoyé par LittleWhite

Code:

---

void ServerPacketHandler::handleRedirect(PacketReader& packet)

---

Possible que cela doit être const PacketReader& mais cela va vous embetter un peu :D

---

Je crois pas, PacketReader fait des modifications de ses variables membres lors de la lecture du string, donc je pense que le const ne doit pas etre la non ?

Citation:

---

Envoyé par LittleWhite

Le fichier ServeurSocket.h a un nombre d'include abérrant (surement trop)
Regardez aussi du coté de la prédéclaration (surement dans la FAQ C++)
(Meme remarque pour le Networker.h)

---

Oui je comprend la predeclaration, c'est mieux que je fasse ca et que je deplace mes include dans le .cpp ?

Citation:

---

Envoyé par LittleWhite

Pourquoi les fichiers de Session sont dans les Tools ?

---

C'est une bonne question, je me suis posé la meme ya quelques mois xD

Citation:

---

Envoyé par LittleWhite

Voilà pour le moment. Il serait génial de générer des diagrammes (Diagramme de classes et de dépendances) pour avoir un plus grand aperçu de l'architecture

---

Je fais ca comment?

Citation:

---

Envoyé par gbdivers

* pareil que LittleWhite, j'aurais aimé avoir quelques chose pour faciliter la compilation (makefile)
* utilisation de C++11 (un peu ;) ) et exceptions -> très bien

---

J'aimerais en fournir un, mais faut que je trouve ou cest dans code blocks ^^
Et pour le C++11, j'aimerais egalement en utiliser plus, mais gcc ne veut pas des nouvelles fonctionnalités de gcc 4.6 :/

Citation:

---

Envoyé par gbdivers

dans main.cpp
* argc et argv non utilisé
* variables "Sis" et "Sls" : tu mets une majuscule habituellement à tes noms de classes et une minuscule à tes noms de variables, sauf là
* tu créés reg dans dès le début, alors que ne l'utilise pas forcement
* tu fais 2 fois la même séquence de code (query + création vecteur + storein), tu devrais factoriser ça peut être
* #include <string> inutile
* #include DatabaseStocks utile ? a priori, c'est pour créer les types ServerLinksStock et ServersLinksStock (d'ailleurs, les noms sont trop proches et peu lisibles = risque de confusion)
* LE PLUS IMPORTANT : je vois des new mais pas des delete -> fuites mémoire. Utilise unique_ptr<> ici (et vire tous les pointeurs nus dans tout le projet a priori)

---

* Je supprive argv & arg ?
* Ca change quoi pour mes variables? comprend pas 8O
* Registry est utilisé dès le debut, si :)
* La sequence de code nexiste plus, j'ai un nouveau fichier main (je crois!)
* String enlevé:roll:
* J'ai besoin des stocks dans le code du main, et oui je sais ils sont proches :(
* Les deletes sont plus loin.. !

Citation:

---

Envoyé par gbdivers

dans Database.h
* tous les include sont nécessaire ?
* toutes les static const : à terme, à mettre dans un fichier de configuration
* c'est quoi BLOB_TRUE et BLOB_FALSE ?
* Database avec sémantique de singletion utile ? utilisé uniquement dans Session.cpp. Voir les nombreuses discussion là dessus sur le forum et sur le blog d'Emmanuel Deloget
* utilité d'avoir la classe en singletion et toutes les fonctions en static ?
* homogénéité des noms de variables (définir les conventions de codage et les respecter :) )
* delete instance -> utiliser unique_ptr<>
* Format(const std::string& sFormat) -> bizarre comme fonction, elle ne semble pas faire un format mais vérifier la présence d'un placeholder dans la chaine
* de plus, pour cette fonction, tu aurais pu faire Format(const std::string& sFormat, std::string s_Placeholder = "%") pour plus de souplesse dans le code
* manque de const aussi dans les fonctions Format
* la seconde fonction Format -> un variadic template 8O on en voit peu. Tu es familier avec le perfect forwarding ou tu as copié collé du code ? :D L'utilisation à l'air correcte. Par contre, je suis pas sur que cela doit être de la responsabilité d'une classe Database de faire cela, tu es sur de respecter le principe de responsabilité unique ? (SRP)
* fonction Exec : tu mélanges plusieurs écriture différentes. Soit ta classe est un singleton et tu dois écrire (comme tu l'as fait) :

Code:

---

1 2	Database* db = new Database(); mysqlpp::Query query = db->Exec(Database::Format("SELECT * FROM `servers` WHERE linkname ='%'", MyServerName));

---

et dans ce cas, Exec() n'a pas de raison d'être static et d'appeler Database::getInstance() en interne.
Soit on doit écrire :

Code:

---

mysqlpp::Query query = Database::Exec(Database::Format("SELECT * FROM `servers` WHERE linkname ='%'", MyServerName));

---

et dans ce cas, oui, Exec() doit être static, mais getInstance() et le constructeur n'ont pas de raison d'être public.
Là, tu autorises les 2 écritures, c'est bof (surtout que tu mets pas ensuite Store() en static donc que la première écriture, d'où risque de confusion pour l'utilisateur, qui peut utiliser 2 écritures pour certaines fonction et 1 seule pour d'autres)
* fonction Link() -> c'est un getter, utilité ? risque de non respect du principe d'encapsulation et exposition de détail interne ?

---

* A voir, je vais reverifier les includes
* Oui, le fichier de conf va venir!
* BLOB * = inutile, j'avais oublié...
* Singleton = En fait je le pensais utile, mais plus tard dans mon code (Vers un fichier pas encore uploadé, je me suis rendu compte qu'il etait inutile ...)
(Bon un seul singleton, c'est pas la mort :D)
* Convention de codage? Kézako ?
* Pour format & query: Oui , c'est vraiment moche, j'ai ecris ca un soir pour tester la connexion a mysql sans repasser derriere. Quand je vais passer au jeu lui meme je vais enormement solliciter mysql et donc je ferais un joli code ! :)

Citation:

---

Envoyé par gbdivers

dans Database.cpp
* Database* Database::instance; -> variable non initialisée
* Database() -> ; en trop après la définition du corps du constructeur

Finalement, ta classe Database n'a pas d'autre fonction que de faire une query. Ca ne serait pas mieux de créer une classe Query et que ta classe Database (ou les 2/3 fonctions de Database) soit interne à Query (détail d'implémentation). Ca serait mieux en terme de sémantique

La suite plus tard...

---

Possible, comme dit plus haut, je dois la reecrire :)

De plus, voici un nouveau zip contenant des mises a jour, vos remarques sont notés en commentaires

Code:

---

// TODO: blabla

---

merci :)
Je posterais une prochaine source avec les corrections.