Why do I get "Segmentation fault (core dumped)" error when trying to implement multithread-CodePudding

I have a main file where I plan to initiate the threads for my c program, for now, I only want to get one of the threads up and running before moving on to the others, but that is proving to be difficult. The purpose of the threads is for a TCP Server and Client to run at the same time, I have already tested my TCP code and it works, the issue now is running each one in its own thread. The following shows my main.cpp code:

#include <thread>
#include <iostream>
#include <functional>

#include "./hdr/tcpip_server.hpp"
#include "./hdr/tcpip_client.hpp"

using namespace std;

tcpServer *backendServer;

//This is done because the callback function of std::thread tcpip_server_thread complains when I only use 'backendServer->Monitor' as my callback function
void StartThread (void) {backendServer->Monitor();}

int main (void) 
{
    /*Initiate and start TCP server thread*/

    std::thread tcpip_server_thread; // done to define object to be used outside the scope of the if statement below

    if (backendServer->Init())
    {
        std::thread tcpip_server_thread (StartThread);
    }

    /*Initiate and start data reader thread*/

    //std::thread tcpip_client_thread (tcpip_client);

    tcpip_server_thread.join();
    //tcpip_client_thread.join();

    return 0;
}

The backendServer class is as follows:

class tcpServer
{
    private:

        int listening;
        sockaddr_in hint;
        sockaddr_in client;
        socklen_t clientSize;
        int clientSocket;
        char host[NI_MAXHOST];      
        char service[NI_MAXSERV];
        char buf[4096];

    public:

           bool Init ();
           void Monitor ();
};

The only error I am getting with this code is the one in the title, and I only get it when the code is executing, no errors are received while compiling the code.

When trying the following:

std::thread tcpip_server_thread (backendServer->Monitor);

I get the following warning:

a pointer to a bound function may only be used to call the function

and

no instance of constructor "std::thread::thread" matches the argument list

Any help would be appreciated as this is my first project implementing threads.

CodePudding user response：

1. Initializing backendServer:

backendServer is a pointer to tcpServer, but it is uninitialized (and does not point to any valid object).
Therefore backendServer->Init(); invokes UB Undefined Behavior, and likely to crash.
If you must use a pointer you must allocate it. Better still use a smart pointer like std::unique_ptr instead.
But in your case I believe the best solution is not to use a pointer at all, and define backendServer as a local variable in main:

int main(void) 
{
    tcpServer backendServer;
    // ...
}

This will require accessing it with backendServer. instead of backendServer->.

2. The thread issue:

At the moment, you have 2 tcpip_server_thread variables. The 2nd one inside the if is shadowing the 1st one you have before.
When you get out of the if's scope, the 2nd tcpip_server_thread will be destroyed, and a std::thread must be joined before destruction. Later on you attempt to join the 1st one which has not even started, causing a 2nd problem.

In order to fix it:

Inside the if, do not declare a new variable. Instead use the one you already have:

tcpip_server_thread = std::thread(StartThread);

If you made backendServer a local in main as suggested above, you can use a lambda that captures it by reference:

tcpip_server_thread = std::thread(
               [&backendServer]() { backendServer.Monitor();});
//--------------^^^^^^^^^^^^^^---------------------------------

Before you join the thread check that it is joinable. In the current code this will not be the case if you didn't enter the if that started the thread:
```
if (tcpip_server_thread.joinable())
{
    tcpip_server_thread.join();
}
```

A side note: Why is "using namespace std;" considered bad practice?.

CodePudding user response：

The main issue of your code is an uninitialised (actually: zero-initialised) pointer:

tcpServer *backendServer;

Note that you never assign a value to! This results in (as a global variable) the pointer being initialised to nullptr, which you dereference illegally later on, e.g. at (the first time during the programme run)

 if (backendServer->Init())

which most likely caused the crash. A quick and dirty fix might look as:

int main()
{
    backendServer = new tcpServer(); // possibly with arguments, depending
                                     // on how your constructor looks like

    // the code you have so far

    delete backendServer; // avoid memory leak!!!
    return 0;
}

You spare all this hassle around manual memory management (-> explicit delete) if you use smart pointers instead, e.g. std::unique_ptr. However unless you possibly want to dynamically exchange the backend server, limit its life-time to anything else than the entire programme run or construct it with arguments that need to be retrieved/calculated within main before (none of appears pretty likely to me in given case) then you most likely are better off with a global object:

tcpServer backendServer; // note the dropped asterisk!

This way the object is created before entering main and correctly destructed after leaving.

As now no pointer any more you now refer to members via . instead of ->, i.e. backendServer.Monitor() for instance.

You actually can construct a std::thread with member function pointers, too. You need, though, to pass the object on which this member function should get called to the thread as well:

std::thread(&tcpServer::Monitor, backendServer);

This works with both functions and objects, the latter are accepted by value, though, thus if you use a global object as recommended above you might rather want to create a pointer:

std::thread(&tcpServer::Monitor, &backendServer);
//                               ^ (!)
// note: NOT if your variable remains a pointer!!!

This way you can actually spare the global variable entirely and create the object within main and the StartThread (actually you should better have named it RunThread) gets entirely obsolete as well.

Alternatives to are converting Monitor function into an operator() or adding such one as

void tcpServer::operator()()
{
    this->Monitor();
}

which makes the object itself callable, thus you could pass it directly to the thread's constructor (std::thread(std::ref(backendServer)); with std::ref preventing the object getting copied) or using a lambda:

std::thread([&]() { backendServer.Monitor(); });

both with the same advantage as providing the member function that you can spare global variable and StartThread function.

Still your code reveals another problem:

if (backendServer->Init())
{
    std::thread tcpip_server_thread(StartThread);
}

You create here a second local variable tcpip_server_thread which, as long as it exists, hides the previous one, but which runs out of scope and thus gets destructed again right after the end of the if-body!

Instead you want to assign the newly created thread to the already existing variable, which would look like:

tcpip_server_thread = std::thread(StartThread);

Actually you get nicer code if you move the entire thread-code into the if block:

// no thread code left here any more

if(backendServer->Init())
{
    std::thread tcpip_server_thread(StartThread);

    // start second thread here, too!

    tcpip_server_thread.join();
}

// no thread code left here any more

Finally you should not join a thread that actually has failed to start. You spot this by checking if the thread is joinable

std::thread tcpip_server_thread (StartThread);
if(tcpip_server_thread.joinable())
{
    // see above for correct construction!
    std::thread tcpip_client_thread(tcpip_client);

    if(tcpip_client_thread.joinable())
    {
        tcpip_server_thread.join();
    }
    else
    {
        // you might need some appropriate error handling like
        // printing/logging a warning message
        // and possibly stop the server thread
    }
}
else
{
    error handling, see above
}

CodePudding user response：

To fix the code I had to do 2 things, one was to not define the tcpServer variable, backendServer, as a pointer, since I never pointed it toward an actual object of the type tcpServer.

Next, I removed the first tcpip_server_thread variable and made sure that the code that initiates ```tcpip_server_thread`` and the code that joins it is in the same scope. In the future, I will implement the std::move function as explained by @wohlstad.

My working code:

#include <thread>
#include <iostream>
#include <functional>

#include "./hdr/tcpip_server.hpp"
#include "./hdr/tcpip_client.hpp"

using namespace std;

/*All the threads*/

tcpServer backendServer;

void StartThread (void) {backendServer.Monitor();}

int main (void) 
{
    /*Initiate and start tcp server thread*/

    if (backendServer.Init())
    {
        std::thread tcpip_server_thread (StartThread);

        if (tcpip_server_thread.joinable())
        {
            tcpip_server_thread.join();
        } 
        else 
        {
            cout << "error";
        }
    }

    return 0;
}