4
\$\begingroup\$

I have a class Tcp_Server that implements a TCP server. This is the base class for the other types of servers available in my program. For example Tcp_Chat_Server. Packet is a struct that we send/receive.

Here's the implementation:

packet.hpp

#ifndef PACKET
#define PACKET
#define MAX_NAME_LEN 24
#include <stdio.h>
struct Packet
{
    Packet();
    ~Packet();
    char name[MAX_NAME_LEN];
    char buf[BUFSIZ];
};
#endif

packet.cpp

#include "packet.hpp"
Packet::Packet() {};
Packet::~Packet() {};

tcp_server.hpp

#ifndef TCP_SERVER
#define TCP_SERVER
#include <netinet/in.h>
#include <sys/select.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <string.h>
#include <stdio.h>
struct server_info
{
    char ip_str[INET_ADDRSTRLEN] = {'\0'};
    int port;
};

class Tcp_Server
{
public:
    Tcp_Server(int port);               // Initializer
    ~Tcp_Server();                      // Destructor
    /***
     * select(), blocking method
     * Return 0 if it's a listening socket.
     * Return 1 if it's a usual socket.
     */
    int WaitingForActions();        
    int NewConnectionHandler();         // If new client. Return socket number.
    virtual int NewMessageHandler() = 0;// This method shouldn't be implement, look at the inheritance classes.
    struct server_info GetServerInfo(); // Fill and return server_info structure.
    int GetCurrentFds();
protected:
    int listen_socket;                  // Listening socket
    struct sockaddr_in serv_addr;       // Structure with information about socket (adress, port etc.)
    struct server_info server_info;     // Information about server in readable form, returned by function.
    fd_set connections;                 // Set of clients
    int max_socket;                     // Number of maximum existing descriptor
    int current_socket;                 // Number of the processed descriptor
};
#endif

tcp_server.cpp

#include "tcp_server.hpp"
Tcp_Server::Tcp_Server(int port)
{
    int param = 1;      // Parametr for setsockopt(), maybe we can change it to bool.
    char ip_str[INET_ADDRSTRLEN] = {'\0'};

    if((listen_socket = socket(AF_INET, SOCK_STREAM, 0)) < 0)
    {
        throw "Can't create a socket!\n";
    }

    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    serv_addr.sin_port = htons(port);
    
    if(setsockopt(listen_socket, SOL_SOCKET, SO_REUSEADDR, &param, sizeof(param)) < 0)
    {
        throw "setsockopt() error!\n";
    }
    
    if(bind(listen_socket, (struct sockaddr*)&serv_addr, sizeof(serv_addr)) < 0)
    {
        throw "bind() error!\n";
    }

    listen(listen_socket, 5);

    FD_ZERO(&connections);
    FD_SET(listen_socket, &connections);
    max_socket = listen_socket;
}

Tcp_Server::~Tcp_Server()
{
    for(int i = 0; i <= max_socket; i++)
    {
        if(i == listen_socket)
        {
            shutdown(i, SHUT_RDWR);
            close(i);
            FD_CLR(i, &connections);
        }
        if(FD_ISSET(i, &connections))
        {
            shutdown(i, SHUT_RDWR);
            close(i);
            FD_CLR(i, &connections);
        }
    }
}

int Tcp_Server::WaitingForActions()
{
    fd_set tmp_set = connections;

    if(select(max_socket + 1, &tmp_set, NULL, NULL, NULL) < 0)
    {
        throw "error in select \n";  
        fprintf(stderr, "error in select \n");
    }

    for(int i = 0; i <= max_socket; i++)
    {
        if(FD_ISSET(i, &tmp_set))
        {
            current_socket = i;
            if(i == listen_socket)  // Если запрос к слушающему сокету
            {
               return 0;
            }
            else
            {
                return 1;
            }
        }
    }
    return -1;
};

int Tcp_Server::NewConnectionHandler()
{
    int new_sock = accept(listen_socket, NULL, NULL);
    if(new_sock < 0)
    {
        throw "accept() error\n";
    }
    else
    {
        FD_SET(new_sock, &connections);
        if(new_sock > max_socket)
        {
            max_socket = new_sock;
        }
    }
    return new_sock;
}

struct server_info Tcp_Server::GetServerInfo()
{
    memset(&server_info, 0, sizeof(server_info));
    inet_ntop(AF_INET, &(serv_addr.sin_addr), server_info.ip_str, INET_ADDRSTRLEN);
    server_info.port = ntohs(serv_addr.sin_port);
    return server_info;
}

int Tcp_Server::GetCurrentFds()
{
    return current_socket;
}

tcp_chat_server.hpp

#ifndef TCP_CHAT_SERVER
#define TCP_CHAT_SERVER
#include "tcp_server.hpp"
#include "packet.hpp"
class Tcp_Chat_Server : public Tcp_Server
{
public:
    Tcp_Chat_Server(int port);
    virtual int NewMessageHandler();
};
#endif

tcp_chat_server.cpp

#include "tcp_chat_server.hpp"
Tcp_Chat_Server::Tcp_Chat_Server(int port) : Tcp_Server(port) {};

int Tcp_Chat_Server::NewMessageHandler()
{
    Packet packet;
    int bytes = recv(current_socket, &packet, sizeof(packet), 0);
    if(bytes <= 0)
    {
        close(current_socket);
        FD_CLR(current_socket, &connections);
        return bytes;
    }
    else
    {
        for(int i = 0; i <= max_socket; i++)
        {
            if(i == current_socket || i == listen_socket)
            {
                continue;
            }
            if(FD_ISSET(i, &connections))
            {
                if((send(i, &packet, sizeof(packet), 0)) <= 0)
                {
                    return -1;
                }
            }
        }
        fprintf(stdout, "%s : %s", packet.name, packet.buf);
        memset(&packet, '\0', sizeof(packet));
    }
    return bytes;
}

So, first of all, I would like to hear about:

  1. Class interfaces;
  2. Hierarchy of classes;
  3. All about virtual things;
  4. New nuances of the network and data transmission over it.
  5. Supportability of this architecture;

Next, and less important:

  1. STL and other libraries
  2. naming

And last, this class is the part of application for instant messaging. At the moment, code is in private repo. It will be interesting to hear your thoughts on how best to design such applications, particularly the process of send/receive messages.

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Everything Jan Schultke said:

Design

I don't see a run() like method. So the user needs to implement that. Is it supposed to look like this:

void run(Tcp_Server& server)
{
    while(!finished) {
        int action = server.WaitingForActions();
        if (action == 0) {
            int socket = server.NewConnectionHandler()
        }
        else if (action == 1) {
            int byteCount = server.NewMessageHandler()
        }
        else {
            // error
        }
    }
}

Which leads to a lot of questions.

  1. Is this the expected pattern to handle it?
  2. What am I supposed to do with the values returned from these two methods?
  • The NewConnectionHandler() already handles the socket stuff.
  • The NewMessageHandler() is virtual so why do I need to write run() and this?
  1. Why is your NewMessageHandler() fiddling with members of the Tcp_Server?
  • Seems like a bad design if you need accesses to the internal members.

Your code should implement that run method and call the NewMessageHandler() when appropriate passing only the socket that needs to be read from to the handler.


Why is this being done on a single thread? Basically when you get a connection now your server blocks all other potential connection attempts while it handles the current message on NewMessageHandler(). This means there is no point using select(). You may as well simply block on accept(), handle the resulting request, then go back to block on accept.

To do this better:

  1. You have a main thread running your version of main().

  2. When the main thread receives a connect or input on any connection it adds a work item to a work queue to be handled separately, and immediately goes back to waiting on select(). Your main thread should do as little work as possible processing data from a connection.

  3. You should have a thread pool. All the threads in the pool should be watching the work queue. If a new item is added to the work queue then they start processing.

  4. If the worker finishes all work on the socket then it closes and goes back to the work queue.

  5. If the worker would block reading or writing to the socket then you put the worker gives the socket back to the main thread so it can wait on the select() and be handled when the socket is ready and the thread goes back to the pool to wait for another piece of work (note: implies state is associated with socket so when you are ready to resume you will need to retrieve the state to continue).


I would design my interface more like this:

class Tcp_Server
{
public:
    Tcp_Server(int port, ThreadPool workers, WorkQueue queue);         
    virtual ~Tcp_Server(); 
    void     run();
private:

    // This function will be called by a worker
    // as defined by the ThreadPool (The thread pool may be empty which
    // means it is called by the main thread).
    //
    // return true to add back to FD_SET to continue listening.
    // return false to indicate connection was closed.
    // Exception: Error on stream. TCP_Server will shut manually close
    //            the connection (any errors will be discarded).
    //            exception will be logged.
    virtual bool NewMessageHandler(int socket) = 0; // Pass socket
};

class ChatServer: public Tcp_Server {
    // Allow the handler to store some state about the connection.
    // Subsequent calls can use this information to continue
    // processing from the last know position (either read or write).
    std::map<int, void*>      socketData;

    // Handler for Chat protocol.
    // Handler should return on closed connection or a potentially blocking
    // read/write.
    virtual bool NewMessageHandler(int socket); // continue handling data.
};

int main() {
    WorkQueue     workItems;
    ThreadPool    workers(5, workItems);          // workers read from workItems.
    ChatServer    server(45, workers, workItems); // server writes to work Items.

    server.run();
}

Code Review

You don't use a namespace.

Add a namespace around your code.


See advice by: Jan Schultke

#define MAX_NAME_LEN 24

See advice by: Jan Schultke

#include <stdio.h>

Don't need constructor/destructor here (they don't do anything). Also this object uses arrays so it is not movable. Not sure how you want to use it. But it may be worth using std::vector<> here and allow the object to be simply moved around your application.

struct Packet
{
    Packet();
    ~Packet();
    char name[MAX_NAME_LEN];
    char buf[BUFSIZ];
};

These include guards are short. And potentially not unique.

#ifndef TCP_SERVER
#define TCP_SERVER

I would add the namespace to the macro guard to make sure that they are unique.


I understand the need for these.

#include <netinet/in.h>
#include <sys/select.h>
#include <arpa/inet.h>
#include <unistd.h>

But these are C libraries.

#include <string.h>
#include <stdio.h>

What are you using them for? Why not use C++ facilities? The C++ std::string class is great (and less error prone to use). The C++ input/output facilities are less error prone (though I believe the C compiler makes this less of a problem nowadays). But you are in C++ so use the C++ std::cout, std::clog and/or std::cerr.


Don't write useless comments.

    Tcp_Server(int port);               // Initializer
    ~Tcp_Server();                      // Destructor

I can see what they are.


Using integer here is a bad policy.

    /***
     * select(), blocking method
     * Return 0 if it's a listening socket.
     * Return 1 if it's a usual socket.
     */
    int WaitingForActions();        

Create an enum and be specific about it. Using the enum allows you to write better self documenting code.


protected was a mistake in the language design. Don't use it. If you must use it, make sure there are only methods in this section to prevent abuse by your derived types. The variables should always be private.

Don't make things available through protected if you don't need to. You should tightly control this to only things that are necessary.

protected:
    int listen_socket;                  // Listening socket
    struct sockaddr_in serv_addr;       // Structure with information about socket (adress, port etc.)
    struct server_info server_info;     // Information about server in readable form, returned by function.
    fd_set connections;                 // Set of clients
    int max_socket;                     // Number of maximum existing descriptor
    int current_socket;                 // Number of the processed descriptor
};

Might be worth including more information in these exceptions.

        throw "Can't create a socket!\n";
        throw "setsockopt() error!\n";
        throw "bind() error!\n";

The user is going to have a hard time correcting the errors from these statements. Add the error number and the specific error message generated. This may lead you to derive an exception class (only do this if the exception allows you to fix the error. Otherwise use a generic exception class).


That print is never reached:

        throw "error in select \n";  
        fprintf(stderr, "error in select \n");

\$\endgroup\$
3
\$\begingroup\$

There are a lot of things to discuss here, so I'll just go from line to line and explain all the issues I see:

#define MAX_NAME_LEN 24

You really shouldn't be using macros in C++ unless it's necessary. They pollute global namespace and have no type safety. Consider constexpr std::size_t MAX_NAME_LENinstead.

#include <stdio.h>

Including C headers is deprecated and they might not even exist depending on the implementation. Use <cstdio> instead.

Packet()

Unnecessary constructor (and destructor). If you define it in the header and give it an empty body in the source file, this means that it will still be called like any function, even though it does nothing. These calls won't be optimized away without link-time optimization. Follow either the rule of zero or rule of five.

char buf[BUFSIZ];

Buffer with implementation-defined size. Each packet will be quite large if you do this, probably 4096 or 8192 bytes. If you really want such a large size, at least use your own instead of making use of a macro from libc.

packet.cpp

This file is not needed once you remove the unnecessary constructor and destructor. You might just want to delete it. Your Packet can be designed as a header-only utility.

~Tcp_Server();

Non-virtual destructor in a virtual class. Your compiler should have probably given you a warning about this already with proper warnings enabled. The problem here is that destroying Tcp_Server will not call the destructor of its derived classes, leading to potential memory leaks. Mark it virtual

virtual int NewMessageHandler() = 0;

It might not be worth making your entire Tcp_Server abstract if the only virtual thing about it is the message handling. Consider using a std::function or a function-pointer callback for message handling which is passed into the class after initialization. This would likely eliminate a lot of boilerplate and would also mean that your server no longer needs to be a virtual class.

throw "error in select \n";

Some people would argue that you should never just throw strings but use the std::exception class hierarchy. Also, why are you throwing in a class that already uses an int result code? Seems like redundant error handling.

\$\endgroup\$
3
  • \$\begingroup\$ The advice is correct. Use <cstdio> not sure the reasoning is true C headers is deprecated. The difference is that the C version puts everything in the global namespace (and optionally in the std namespace). While the C++ headers puts everything in the std namespace (and optionally in the global namespace). \$\endgroup\$ Mar 8, 2021 at 16:03
  • \$\begingroup\$ Error codes are great for tracking issues inside your components. But error codes should not be allowed to cross interface boundaries (because people forget to check). Exceptions are great for passing issues that can not be handled locally. If you need to consider the issue in relation to a broader context exceptions are a better choice. \$\endgroup\$ Mar 8, 2021 at 16:11
  • \$\begingroup\$ When changing macros to properly typed constants, we should also change the names, too, so we retain attention on the real macros that still need careful handling. \$\endgroup\$ Mar 8, 2021 at 17:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.