TCP socket server for UNIX

Question

I'm very new to C++, but I really want to write good code and increase my development skill, so I ask you for some review.

Scheme of socket server

This is the scheme of how my socket server works. It creates a socket, binds it, sets to listen state, creates a new thread (in order not to block the execution of the program), which accepts new connections, creates new threads for each client, reads data from them and dispatches data to observers (server is a child of Observable class, which allows to dispatch events to Observers.

#include <iostream>
#include <string.h>
#include <stdio.h>
#include <vector>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <pthread.h>
#include "Observer.h"
using namespace std;
class SocketServer : public Observable
{
 private:
 string serverIp;
 int serverPort;
 int masterSocket;
 void SocketCreate ( );
 void SocketBind ( );
 void SocketListen ( );
 static void* SocketAccept ( void* );
 static void* SocketRead ( void* );
 void RequestDispatch ( int , string );
 public:
 SocketServer ( string , int );
 static void Send ( int , string );
 static void Log ( int , string );
};
struct ClientRequest
{
 int socketFD;
 string request;
};
struct ServerAndSocket
{
 SocketServer* socketServer;
 int socketFD;
};
 SocketServer::SocketServer ( string serverIp , int serverPort )
{
 this->serverIp = serverIp;
 this->serverPort = serverPort;
 this->SocketCreate();
 this->SocketBind();
 this->SocketListen();
 pthread_create ( new pthread_t() , NULL , SocketServer::SocketAccept, this );
};
void SocketServer::SocketCreate ( )
{
 this->masterSocket = socket ( AF_INET , SOCK_STREAM , 0 );
 if ( this->masterSocket < 0 )
 {
 perror ( "socket" );
 _exit(0);
 }
 else
 {
 int opt = 1;
 setsockopt (this->masterSocket, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
 cout << "Socket created successfully with file descriptor " << this->masterSocket << "\n";
 };
};
void SocketServer::SocketBind ( )
{
 struct sockaddr_in serverAddress;
 serverAddress.sin_family = AF_INET;
 serverAddress.sin_port = htons ( this->serverPort );
 serverAddress.sin_addr.s_addr = inet_addr ( this->serverIp.data());
 if ( bind ( this->masterSocket , (sockaddr*)&serverAddress , sizeof ( serverAddress ) ) < 0 )
 {
 perror ( "bind" );
 _exit(0);
 }
 else
 {
 cout << "Socket bound successfully to the " << this->serverIp << ":" << this->serverPort << " address\n";
 };
};
void SocketServer::SocketListen ( )
{
 listen ( this->masterSocket , 5 );
 cout << "Socket is beeing listened now\n";
};
void* SocketServer::SocketAccept ( void* serverPointer )
{
 int socketFD;
 pthread_t* clientThread;
 ClientRequest clientRequest;
 SocketServer* this_;
 ServerAndSocket serverAndSocketPtr;
 this_ = (SocketServer*)serverPointer;
 while ( 1 )
 {
 socketFD = accept ( this_->masterSocket , NULL , NULL );
 if ( socketFD < 0 )
 {
 perror ( "accept" );
 }
 else
 {
 this_->RequestDispatch ( socketFD , "CLIENT_CONNECTED" );
 serverAndSocketPtr.socketServer = this_;
 serverAndSocketPtr.socketFD = socketFD;
 pthread_create ( new pthread_t() , NULL , SocketServer::SocketRead, &serverAndSocketPtr );
 };
 };
};
void* SocketServer::SocketRead ( void* serverAndSocketPointer )
{
 char input[256];
 int inputLength;
 SocketServer* socketServerPtr;
 int socketFD;
 ServerAndSocket* serverAndSocketPtr;
 serverAndSocketPtr = (ServerAndSocket*)serverAndSocketPointer;
 socketServerPtr = serverAndSocketPtr->socketServer;
 socketFD = serverAndSocketPtr->socketFD;
 while ( 1 )
 {
 memset ( (void*)&input , '0円' , sizeof ( input ) );
 inputLength = read ( socketFD , (void*)&input , 255 );
 if ( inputLength < 0 )
 {
 perror ( "read" );
 }
 else if ( inputLength == 0 || input[0] == '0円' )
 {
 socketServerPtr->RequestDispatch ( socketFD , "CLIENT_DISCONNECTED" );
 pthread_exit( NULL );
 }
 else
 {
 socketServerPtr->RequestDispatch ( socketFD , input );
 };
 };
};
void SocketServer::RequestDispatch ( int socketFD , string request )
{
 struct ClientRequest clientRequest;
 SocketServer::Log ( socketFD , "---> " + request );
 clientRequest.socketFD = socketFD;
 clientRequest.request = request;
 this->DispatchEvent ( (void*)&clientRequest );
};
void SocketServer::Send ( int socketFD , string message )
{
 int bytesWritten;
 bytesWritten = write ( socketFD , message.c_str() , message.size() + 1 );
 if ( bytesWritten < 0 )
 {
 perror ( "write" );
 }
 else
 {
 SocketServer::Log ( socketFD , "<--- " + message );
 };
};
void SocketServer::Log ( int socketFD , string message )
{
 sockaddr address;
 socklen_t addressLength;
 sockaddr_in* addressInternet;
 string ip;
 int port;
 getpeername ( socketFD , &address , &addressLength );
 addressInternet = (struct sockaddr_in*)&address;
 ip = inet_ntoa ( addressInternet->sin_addr );
 port = addressInternet->sin_port;
 cout << ip << ":" << port << " " << message << "\n";
};

A year after this code grew to a Object-oriented Linux networking library

• Loki Astari: Using a thread per connection is not very scalable. Its OK if your expect your connection count to be in the low teens but after that not so much. Have a look at select() as a starting point epoll() is a natural extension to select() but takes a bit more work. It allows you to have one thread service all connections simultaneously.
• hroptatyr: you forgot that ports in sockaddr structs are network byte-order, so use ntohs() in the Log() method
• user2813274: @LokiAstari could I ask for a reference where using a thread per connection fails to scale past the teens? afaik performance will stop scaling once you go beyond your CPU core count and memory may become an issue going past 2k (2gb), but I don't see why such a relatively low number would cause issues
• Loki Astari: @user2813274: The apache blog would be a good place to start. Then do some research into the C10k problem. That should find you enough references on the issue.

Answer 1

#include <iostream>
#include <string.h>
#include <stdio.h>
#include <vector>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <pthread.h>
#include "Observer.h"
using namespace std;

This is fine for trivial demo code and such, but more often a mistake for anything larger or more serious.

class SocketServer : public Observable {

Personally, I think I'd prefer this as something like:

namespace Socket {
 class Server {
 // ...
 };
}

---

 void SocketCreate();
 void SocketBind();
 void SocketListen();
 static void* SocketAccept(void*);
 static void* SocketRead(void*);

...then these would become just : Create, Bind, Listen, Accept, and Read (but the full names would be, for example, Socket::Server::Create, remaining quite self-explanatory).

SocketServer::SocketServer(string serverIp, int serverPort)
{
 this->serverIp = serverIp;
 this->serverPort = serverPort;

A member initializer list is generally preferable:

Server::Server(string serverIp, int serverPort) : serverIp(ServerIp), serverPort(serverPort) {

---

 this->SocketCreate();
 this->SocketBind();
 this->SocketListen();

this-> is ugly noise. Avoid it.

 pthread_create(new pthread_t(), NULL, SocketServer::SocketAccept, this);

If at all possible, you almost certainly want to use C++11's built-in thread support instead of using pthreads directly. The result is not only much more portable, but nearly always quite a bit cleaner as well.

void SocketServer::SocketCreate()
{
 this->masterSocket = socket(AF_INET, SOCK_STREAM, 0);
 if (this->masterSocket < 0)

One more time: this-> is ugly noise that should be avoided whenever possible (and at least in C++, that's nearly always).

 {
 perror("socket");
 _exit(0);
 }

In my opinion, this is a terrible idea. Code at this level shouldn't set policy about how errors are handled this way. It should (probably) retrieve the error message string using strerror, then throw an exception.

 else
 {
 int opt = 1;
 setsockopt(this->masterSocket, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
 cout << "Socket created successfully with file descriptor " << this->masterSocket << "\n";

Again, using cout for something on the order of logging seems a rather poor idea.

 if (bind(this->masterSocket, (sockaddr*) &serverAddress, sizeof (serverAddress)) < 0)
 {
 perror("bind");
 _exit(0);
 }
 else
 {
 cout << "Socket bound successfully to the " << this->serverIp << ":" << this->serverPort << " address\n";
 };

I'll note what a poor idea this is one last time, and leave it at that.

 char input[256];

This size should probably be set with a named constant instead of a magic number.

memset((void*) &input, '0円', sizeof (input));

This seems to be pointless.You gain nothing from setting this memory to 0's immediately before read puts its data into the

 inputLength = read(socketFD, (void*) &input, 255);

This should almost certainly use something like sizeof(input)-1 instead of 255.

Answer 2

Something I hadn't seen anyone else pick up on which I think deserves mention is in regards to your Send method implementation not handling incomplete writes. According to a manual page for the write system call (and experience) you are not guaranteed that bytesWritten will be all of the bytes that you've requested: "It is not an error if this number is smaller than the number of bytes requested".

This can be remedied by adding code in your Send method to loop over the writing of the data. Something in C++11 or newer that might look like:

void SocketServer::Send(int socketFD, string message)
{
 auto bytesToSend = message.length();
 auto messagePtr = message.c_str();
 while (bytesToSend > 0)
 {
 const auto bytesWritten = write(socketFD, messagePtr, bytesToSend);
 if (bytesWritten == -1)
 {
 // do your error handling and possibly return depending on errno.
 }
 else
 {
 bytesToSend -= bytesWritten;
 messagePtr += bytesWritten;
 }
 }
}

Answer 3

I think you should probably add a few destructors (think RAII) so both the open socket and the listening sockets always get properly closed.

Oh, and while Jerry's comments are pretty good, ignore the one about using

this->

Its not 'noise', its actually a good practice according to several coding guidelines I've worked with. Your compiler doesn't need it, but it does make your code more readable to other project members.

• Loki Astari: Using this-> is not idiomatic C++. Also if you need to use it because you have shadowed variables your code is actually more brittle (because if you forget to use it then you are not doing what you expect). It is better to not use and turn on warnings and the compiler will tell when you are code is breaking.