Select() server implementation

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  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 & server,
                             Client & 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
#include
#include
#include
#include

#include "ServerSSL.h"

template class Client;
template class Server;

template
using callback = void (*)( Server & server, Client & client );

template
class Server {

private:

bool _working;

SOCKET _listenFd;
int

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 
  using callback = void (*)( Server & server, Client & 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 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::value, "Must use POD type for packet");

Rule of three/Five

If you do memory management in your class. Then yo

StackExchange Code Review Q#111310, answer score: 3