Select() server implementation

Question

My task was to write a server class, which uses select(), for a student projet.
It was working, but i'm now willing to improve it a lot, and be able to use it in other personnal projects, so any critique and improvement idea is welcomed !

Here is the main.cpp :

// example packet
typedef struct packet {

    char       msg[ 4096 ];

} packet;

int                 main( int ac, char * av[] )
{
  packet            shutdownPacket = { "QUIT" };

  Server< packet >  server;

  if ( ac != 2 ) {
    cerr << "Usage : ./server port" << endl;
    return ( EXIT_FAILURE );
  }

  try {

    server.bindSock( av[1] );
    server.listenPort();
    server.setDefaultOnReadCallback( & OnReadCallback ); // this one will be detailed beyond
    server.setOnShutdownPacket( shutdownPacket ); // this packet will be sent to all clients on server quit
    server.startServer();

  } catch ( const exception & e ) {
    cerr << e.what() << endl;
  }

  return ( EXIT_SUCCESS );
}

The function pointer passed to setDefaultOnReadCallback() is used to define the behavior when activity is detected on the client file descriptor, here is an example that will just forward the packet to every other clients :

void         OnReadCallback( Server< header > & server,
                             Client< header > & client )
{
    auto       list = server.getClientList();
    packet   * clientPacket = server.getPacket();

    for ( size_t i = 0; i < list.size(); i++ ) {

        if ( list[i].fd() != client.fd() )
           server.send( list[i], clientPacket, server.getPacketSize() );

    }       
}

And here is the server implementation :

  #pragma once

  #include <vector>
  #include <string>
  #include <stdexcept>
  #include <iostream>
  #include <algorithm>

  #include "ServerSSL.h"

  template <class T> class    Client;
  template <class T> class    Server;

  template <typename T>
  using callback = void (*)( Server<T> & server, Client<T> & client );

  template <class T>
  class    Server {

  private:

    bool                   _working;

    SOCKET                 _listenFd;
    int                    _fdMax;
    vector< Client< T > >  _selectFds;

    fd_set                 _initFds;
    fd_set                 _readFds;

    T                    * _packet;
    T                      _onShutDownPacket;
    pSize                  _packetSize;

    ServerSSL              _ssl; // i won't detail the implementation here but the server uses SSL

    callback<T>            _defaultOnReadCallback;

  public:

    Server()
      {
        _working = true;
        _listenFd = INVALID_SOCKET;
        _packetSize = sizeof( T );
        _packet = new T;
        _defaultOnReadCallback = NULL;
        memset( _packet, 0, _packetSize );
        memset( & _onShutDownPacket, 0, _packetSize );
        FD_ZERO( & _initFds );
      }

    ~Server()
      {
        delete _packet;
      }

    void                  bindSock( const string & port )
    {
      struct addrinfo       *res;
      struct addrinfo       *tmp;
      struct addrinfo       hints;
      SOCKET                sockfd;
      int                   err;

      memset( & hints, 0, sizeof( struct addrinfo ) );
      hints.ai_family = AF_UNSPEC;
      hints.ai_socktype = SOCK_STREAM;
      hints.ai_protocol = IPPROTO_TCP;
      hints.ai_flags = AI_PASSIVE;

      size_t p;
      try {
         p = stol( port );
      } catch ( const exception & e ) {
         p = 1664;
      }

      err = getaddrinfo( NULL, to_string( p ).c_str(), & hints, & res );
      if ( err != 0 ) {
        freeaddrinfo( res );
        throw runtime_error( gai_strerror( err ) );
      }

      for ( tmp = res; tmp != NULL; tmp = tmp->ai_next )
        {
           sockfd = socket( tmp->ai_family, tmp->ai_socktype, tmp->ai_protocol );
           if ( sockfd != INVALID_SOCKET ) {
             if ( bind( sockfd, tmp->ai_addr, tmp->ai_addrlen ) == 0 ) {
                break;
              }
            }
           closesocket( sockfd );
        }

      freeaddrinfo( res );
      if ( tmp == NULL )
        throw runtime_error( "Unable to bind socket" );

      _listenFd = sockfd;
    }

    void                  listenPort( const int backlog = 42 )
    {
      if ( listen( _listenFd, backlog ) == SOCKET_ERROR )
        throw runtime_error( neterror( errno ) );
    }

    void                  startServer( void )
    {
      FD_SET( _listenFd, & _initFds );
      _fdMax = _listenFd;
      _selectFds.push_back( Client<T>( _listenFd ) );
      for ( ; _working ; )
        {
           _readFds = _initFds;
           if ( select( _fdMax + 1, & _readFds, NULL, NULL, NULL ) == -1 ) {
               notifyServerQuit();
               throw runtime_error( neterror( errno ) );
           }
           for ( size_t i = 0; i < _selectFds.size(); i++ ) {

              if ( FD_ISSET( _selectFds[i].fd(), & _readFds ) ) {
                 if ( _selectFds[i].fd() == _listenFd )
                     acceptClient();
                 else
                     onReadCallback( _selectFds[i] );
              }
           }
        }
      notifyServerQuit();
    }

    void                  shutdown( void )
    {
      _working = false;
    }

    void                  setOnShutdownPacket( T packet )
    {
      _onShutDownPacket = packet;
    }

    void                  setDefaultOnReadCallback( const callback< T > defaultCallback )
    {
      _defaultOnReadCallback = defaultCallback;
    }

    SOCKET                fd( void ) const
    {
      return _listenFd;
    }

    T *                   getPacket( void ) const
    {
      return _packet;
    }

    pSize                 getPacketSize( void ) const
    {
      return _packetSize;
    }

    vector< Client< T > > getClientList( void ) const
      {
         return _selectFds;
      }

    int                   send( const Client<T> & client, void * packet, const size_t size ) const
    {
      return _ssl.send( client.ssl(), packet, size );
    }

    int                   recv( const Client<T> & client, void * packet, const size_t size ) const
    {
      return _ssl.recv( client.ssl(), packet, size );
    }

  private:

    void                  acceptClient( void )
    {
      SOCKADDR_STORAGE    addr;
      SOCKET              nsockfd;
      socklen_t           addrlen;

      addrlen = sizeof( addr );
      memset( & addr, 0, addrlen );
      nsockfd = accept( _listenFd, ( SOCKADDR * ) & addr, & addrlen );

      if ( nsockfd == SOCKET_ERROR ) {
        cerr << "accept " << neterror( errno ) << endl;
        return ;
      }

      _selectFds.push_back( Client< T >( nsockfd, _ssl.new_connexion(), _defaultOnReadCallback ) );
      FD_SET( nsockfd, & _initFds );
      if ( nsockfd > _fdMax )
        _fdMax = nsockfd;

      cout << "Connection accepted" << endl;
    }

    void                  onReadCallback( Client<T> & client )
    {
      memset( _packet, 0, _packetSize );
      int len = _ssl.recv( client.ssl(), _packet, _packetSize );

      if ( len <= 0 )
        closeConnection( len, client );
      else
        client.work( *this, client );
    }

    void                  closeConnection( const size_t recvRet, const Client<T> & client )
    {
      if ( recvRet == 0 )
        cout << "Connection closed" << endl;
      else {
        cerr << "recv " << neterror( errno ) << endl;
      }

      if ( closesocket( client.fd() ) == SOCKET_ERROR )
        cerr << "close " << neterror( errno ) << endl;
      _ssl.delete_connexion( client.ssl() );

      FD_CLR( client.fd(), & _initFds );
      _selectFds.erase( remove_if( _selectFds.begin(),
                        _selectFds.end(),
                        [client] ( const Client<T> & cl )
                        { return cl.fd() == client.fd(); } ),
                       _selectFds.end() );
    }

    void                  notifyServerQuit( void )
    {
      cout << _onShutDownPacket.cmd << endl;
      for ( size_t i = 0; i < _selectFds.size(); i++ ) {

        if ( _selectFds[i].ssl() ) {
           _ssl.send( _selectFds[i].ssl(),
                      & _onShutDownPacket,
                      _packetSize );
           _ssl.delete_connexion( _selectFds[i].ssl() );
        }

        if ( closesocket( _selectFds[i].fd() ) == SOCKET_ERROR )
            cerr << "close " << neterror( errno ) << endl;

      }
    }

  };

  /*
  ** CLIENT CLASS
  */

  template <class T>
  class    Client {

  private:

    SOCKET    _fd;
    SSL     * _ssl;

    callback<T>  _defaultOnReadCallback;

  public:

    Client( const SOCKET fd, SSL * ssl = NULL, const callback<T> job = NULL ) : _fd( fd )
      {
        _defaultOnReadCallback = job;
        setOnReadCallback( job );
        _ssl = ssl;
        if ( _ssl != NULL ) {
           SSL_set_fd( _ssl, _fd );
           if ( SSL_accept( _ssl ) == -1 )
              throw runtime_error( ERR_reason_error_string( ERR_get_error() ) );
        }
      }
    ~Client() {}

    SOCKET    fd( void ) const
    {
      return _fd;
    }

    SSL *     ssl( void ) const
    {
      return _ssl;
    }

    void      setOnReadCallback( const callback<T> task = NULL )
    {
      if ( task == NULL )
        work = _defaultOnReadCallback;
      else
        work = task;
    }

    callback<T>  work;

  };

Answer 1

Low level networking code is very tricky

Unfortunately with networking, all kinds of things can and will go wrong. For example you may encounter a SIGPIPE which will asynchronously crash your program. See this question and the answers for a discussion on how to handle this.

Also you have no guarantee that you receive a full packet at once, but this is hidden in your ssl implementation, so I can't really comment.

You seem to have spent some thought or debugging on error handling, which is a good start. But in the current state I cannot really tell whether your error handling is otherwise sufficient.

Considering that networking code is especailly ugly and hard to get right, I would advise to use an established library for production code, such as boost::asio. Unfortunately even those seem to not always take care of the SIGPIPE.

Make use of inheritance

Instead of setDefaultOnReadCallback you can make a pure virtual method in Server and inherit the specific server to implement the read callback. That will result in much leaner code.

Also reconsider the parameters of the callback method to include the actual packet instead of having to ask the server which will begin to fail horribly if you run asyonchronously.

Encapsulate the packet

For example your packet is not always complete. It seems rather dangerous to allow for abitrary types as packets but then dont guarantee that they are transmitted completely. An alternative would be to only provide the max packet size as template parameter and then ecapsulte the packet data / size into a vector<char> like type.

You should also avoid to copy around the packet. You can either delete the copy ctor and assignment operator, or make it private and provide an explicit version.

Avoid ambigous names

vector< Client< T > > _selectFds; is a really bad name.

list = server.getClientList();

should be

clients = server.getClients();

Or omit the get consistently.

Avoid raw loops

Rewrite raw loops as algorithms or range loops

Instead of:

for ( size_t i = 0; i < list.size(); i++ ) {
        if ( list[i].fd() != client.fd() )
           server.send( list[i], clientPacket, server.getPacketSize() );

use

for ( auto& otherClient : clients ) {
    if ( otherClient.fd() != client.fd() )
       server.send( otherClient, clientPacket, server.getPacketSize() );

Pass/return by (const) reference when appropriate

Don't copy around big objects when you don't need to. That applies to packet and all vectors for performance reasons and IMHO also to Client for logical reasons. I wouldn't want networking code where client objects are copied around and then you may end up with inconsistent state.

Use sane interfaces

Don't give the port as string to the server.

  try {
     p = stol( port );
  } catch ( const exception & e ) {
     p = 1664;
  }

This code should not be in the server. It wouldn't be a very generic server if it happens to use a hidden default port.

It would also be better to catch std::locic_error here.

Use std::...

You even forgot the terrible using namespace std;. Anyway don not do it. Use std:: for all appropriate symbols.

Fix your indentation

Your formatting is really off. You also have way too many redundant newlines.

Make sure your cleanup properly

I think you leak file descriptors / open sockets considering you don't do anything the like int he destructor. Consider using more RAII in your code to make those errors more obvious.

Answer 2

In Main

I see nothing major.
So these are all very minor.

Typedef on struct not needed in C++

typedef struct packet {

    char       msg[ 4096 ];

} packet;

The typedef is not needed here. In C it was needed because structure names where in a separate namespace (Its called something else). But in C++ they changed that so that struct were put in the same namespace as other identifiers.

Long white space breaks

int                 main( int ac, char * av[] )

This is annoying (but not wrong). Keep your white space consistent. It will help other developers read your code if it is spaced the same as other code.

argc/argv

int                 main( int ac, char * av[] )

The parameters to main() are traditionally called argc and argv.

Try/Catch in main

int main(int argc, char* argv[])
{
  try {

  } catch (exception const& e) {
    cerr << e.what() << endl;

    // I add the following.
    throw; // Re-throws the current exception.
  }

  return ( EXIT_SUCCESS );
}

This is a good pattern. You should always catch in main (to make sure the stack is forced to unwind). You should also consider re-throwing the exception. That way the OS will know that the application exited because of an exception.

In The Server

Callback functions!

Callback functions are very C like.

  template <typename T>
  using callback = void (*)( Server<T> & server, Client<T> & client );

Even when you use callbacks you should also allow the user to pass a void* parameter to store state for the callback. Without it callbacks become pretty useless (as you can not determine who the callback is for).

But saying that; it is more traditional to pass functors in C++. This allows the callback and the state to combined into a single object.

    void                  setDefaultOnReadCallback( const callback< T > defaultCallback )
    {
        _defaultOnReadCallback = defaultCallback;
    }

So normally I would expect the register function to be fully templatized and allow me to pass any object to act as the action.

Layout of types.

IT is traditional in C for * to be put on the right with the variable. In C++ it is traditional for the * to be put on left by the type. This is because the type is very very important and you want all the type information in a single place.

T                    * _packet;
// More Traditional to write
T*                     _packet;

Prefer to use initializer lists

The initializer list is there even if you don't explicitly specify it. It will use the default constructor for objects that have it. So if you don't use it you are first constructing the object then calling that objects assignment operaotr in the body.

    Server()
      {
        _working = true;
        _listenFd = INVALID_SOCKET;
        _packetSize = sizeof( T );
        _packet = new T;
        _defaultOnReadCallback = NULL;
        memset( _packet, 0, _packetSize );
        memset( & _onShutDownPacket, 0, _packetSize );
        FD_ZERO( & _initFds );
      }

Avoid this double initialization by always just using the initializer list.

Also make sure that all variables are initialized in the constructor. Currently you don't initialize:

_fdMax          // int indeterminate value. UB to read
_selectFDs      // Sure that's OK because it has a default constructor.
_readFDs        // This is a C class and has no constructor. So its value is indeterminate. UB to read
_packedtSize    // I have not idea with the type `pSize` is (but a POD will not initialize itself)
_ssl            // Again no idea what the type is but looks like it may have a default constructor.

Anyway because I can't find theses type easily it means a maintainer will have the same issues. Make sure they all got initialized.

Select()
    : _working(true)
    , _listenFd(INVALID_SOCKET)
    , _fdMax(0)
    , _selectFds()
    , _initFds()
    , _readFds()
    , _packet(new T)
    , _onShutDownPacket()
    , _packetSize(0)
    , _ssl()
    , _defaultOnReadCallback(nullptr)
{
        memset( _packet, 0, _packetSize );
        memset( & _onShutDownPacket, 0, _packetSize );
        FD_ZERO( & _initFds );
}

This is so dangerious. DO NOT DO THIS

        memset( _packet, 0, _packetSize );
        memset( & _onShutDownPacket, 0, _packetSize );

You are wipping memory that belongs to a type. This works if your type is real simple (ie POD). But any other type has a constructor/destructor to handle initialization. This should never be done to C++ classes. This is a real bad hangover from your C days. You should let the type T know how to construct itselef from a packet or make the creator of the object provide some form of translator so it is not built into your server.

If you want to do this, you need to add lots of static asserts to your code to validate this is not dangerous.

        static_assert(std::is_pod<T>::value, "Must use POD type for packet");

Rule of three/Five

If you do memory management in your class. Then your object owns the object.

    ~Server()
      {
        delete _packet;
      }

This means you should be defining all three compiler generated methods. You have forgotten to define the assignment operator operator=() and copy constructor Server(Server const&) which means the following will break your code.

   Server<X>   x;
   Server<X>   y(x);   // broken
   Server<Z>   z;

   z = x;              // broken (it compiles though).

You are basically leaving yourself open to a double delete.

*BUT Adding these functions is the wrong solution.

There is a rule called Separation of Concerns. This basically states your object should do either business logic or resource management. But it should never do both. Since most of your class is business logic (the business here is network traffic management) then I assume you want to delicate resource management to another class. Luckily there is a standard C++ class designed explicitly for this.

    std::unique_ptr<T>  _packet;

In the constructor:

    Server()
      : ....
      , _packet(new T)
      ......
    {}

In the destructor do nothing. As the unique_ptr class will handle the resource management for you. Also because unique_ptr does not have an assignment operator or copy constructor it will not allow your class to have these either. SO all the problems solved in one place.

The short rule is that you should NEVER be doing manual resource management as there is a standard class that will do it (OK sometimes you do want to do it but rareily).

This means there should be zero pointers in your code. When you do have pointers they are rare and it should be well documented the ownership semantics that they use.

Why are you passing the port as a string

    void                  bindSock( const string & port )

C++ is highly types. A port number is not a string its a number. Pass it as such.

There is an implied order in your calls.

You need to call bindport() then listenPort(). This is very C like and forces the user to understand the class more than they should. You should write the code so that it can not be abused.

Why not pass the port as a parameter to the constructor. You obviously need a port before the object is valid. So this logically implies that the port is part of the construction of the object.

Select is very old

Yes it works. But it has several known issues and does not work well with lots of incoming connections (like you get in WebServers). Google the C10K problem.

There are a couple of more modern libraries out there you should check out.

• poll
• epoll
• others I forget.

But if you really want to avoid the problem look at libevent. It provides a wrapper around all these low level network library and uses the fastest one for your architecture.

Answer 3

Actually, I think you want to use poll() rather than select() but this is besides the point. To write to multiple clients you want to make sure that you don't use blocking writes. You might get away with not detecting whether a particular can digest more data and thus you might not need to use poll() (or select()) when writing. However, if you want to be prepared for clients consuming data with different speed you might want to receive notifications when a client can digest more data.

Other than that, poll() or select() can notify on both data available in the read buffer or space available in the write buffer. The use is pretty much the same except that you need to set a different flag.

Your code is well-made, but there is something you could do to make it more readable and more efficient.

Coding-Style

• int main( int ac, char * av[] ) - my eyes hurt! I suggest you to not do this: if you are trying to make it more readable, you're doing it in the wrong way.

• #pragma once directs the preprocessor of MS Visual C++ to #include the file only one time per compilation unit, even if more than one #include for the file is encountered. #pragma is the standard way to add non-standard behavior to C++; other compilers will just ignore the line. A better way to achieve the effect would be:

xyz.h

#ifndef _XYZ_H_INCLUDED
#define _XYZ_H_INCLUDED
.... /* remainder of file */
#endif

Visual C++ excludes rarely used stuff from the platform specific.

#pragma once can fail if the file system contains links to the same file that the compiler cannot follow.

For example, NTFS5 (Windows) supports hard links, but you can't detect that files are linked if you access the file system across an (SMB) network. What about to use a standard #ifndef include guard? It works just fine. The one caveat is that GCC didn't support it before version 3.4.

---

Other imperfections

• int main( int ac, char * av[] )

Originally and traditionally parameters name are: int argc, char **argv. Why did you rename them?

• A member function is a function that need a class instance to be called on. Members function cannot be called without providing the instance to call on to. That makes it harder to use sometimes.

A static function is almost like a global function: it doesn't need a class instance to be called on. So you only need to get the pointer to the function to be able to call it.

Take a look to std::function (or std::tr1::function or boost::function if your compiler doesn't provide it yet), it's useful in your case as it allow you to use anything that is callable (providing () syntax or operator ) as callback, including callable objects and member functions (see std::bind or boost::bind for this case).

Functions aren't comparable, but pointers are. The reason functions aren't comparable is that there is no way to determine if functions are equal (not just in C++, in computer science). I.e., there is no way to determine if functions have the same value. However, by using pointers, we can at least see if they occupy the same space in memory.

bind. Take care when using this, and you may have to perform your own checks when a callback is registered or before you call bind, to make sure that you don't have two duplicate binds of the same function but that occupy different places in memory.

To use a callback to customize a class instead of subclassing, with your alternative you can:

• Change behavior of an object on the fly (just by assigning a new object)

• Mix and match behavior in classes that have more than one of these, and conversely never enforce that two related implementations are always used together in such a class.

• Essentially make every instantiation of the class a subclass.

However, the function objects can not access protected members of "their" object, which may require that you break encapsulation on the host class so that the function objects can do what they need to do.

All in all, this seems like a good way to associate an object with external behavior that should be initiated by that object (e.g., a button click handler), but using it to extend the inherent behavior of a class seems like it creates more problems that it solves.

The most common example of a good use of this paradigm is for callback functions or event handlers, because the class is concerned with the mechanics of drawing the button and handling mouse events (for a button class), or with managing the network protocol (for a network socket class). In these cases, making the event handler virtual and implementing it in a subclass isn't good object-oriented design because the subclasses aren't conceptually new "types" of their parent classes, just like you shouldn't have to subclass a button to change its height and width.

• Bad naming for variables: vector< Client< T > > _selectFds; - someone who reads this ask himself: "What am I reading?"

• You used the cross-platform of EXIT_SUCCESS and EXIT_FAILURE, that's good: as it's delinated to C99+ and C++ standards, you shouldn't explicitly return 0; from main(). It's good to consider to use EXIT_SUCCESS and EXIT_FAILURE if you decide to return explicitly from main().

• Avoid raw-loops: they are an anti-pattern modern C++, it makes code hard to analyze.