8

I am playing around with some sockets, thread and mutexes. My question concerns threads and mutexes:

int ConnectionHandler::addNewSocket(){

    this->connectionList_mutex.lock();
    std::cout << "test1" << std::endl;
    this->connectionList_mutex.unlock();

    return 0;
}

int ConnectionHandler::main(){
    while(true){
        this->connectionList_mutex.lock();
        std::cout << "test2" << std::endl;
        this->connectionList_mutex.unlock();
    }

}`

The main function is running in one thread, while the addNewSocket is called by another thread. The problem is, that when addNewSocket is called once (by the second thread), the next unlock by thread 1 (main) will fail with a strange "signal SIGABRT". I have worked two days on this now, but i did not manage to get it fixed, sadly. I hope you can help me.

Edit: ConnectionHandler is a class, that has connectionList_mutex as a member.

Edit: Sometimes i also get this error: "Assertion failed: (ec == 0), function unlock, file /SourceCache/libcxx/libcxx-65.1/src/mutex.cpp, line 44." but it occurs randomly.

Edit: This is the whole class (Reduced to a minimum, should be context independant to a certain degree, but crashes when i put it right after a client connected, and works if i put it right after the start:

class ConnectionHandler{
public:
    ConnectionHandler();
    int addNewSocket();
private:
    int main();
    static void start(void * pThis);

    std::mutex connectionList_mutex;
};

ConnectionHandler::ConnectionHandler(){
    std::thread t(&this->start, this);
    t.detach();
}
void ConnectionHandler::start(void * pThis){
    ConnectionHandler *handlerThis;
    handlerThis = (ConnectionHandler *)pThis;
    handlerThis->main();
}


int ConnectionHandler::addNewSocket(){

    this->connectionList_mutex.lock();
    std::cout << "test1" << std::endl;
    this->connectionList_mutex.unlock();

    return 0;
}

int ConnectionHandler::main(){
    while(true){
        this->connectionList_mutex.lock();
        std::cout << "test2" << std::endl;
        std::this_thread::sleep_for(std::chrono::milliseconds(100));
        this->connectionList_mutex.unlock();

    }

}
Jonas Eschmann
  • 457
  • 3
  • 9
  • Why tag std? Are your mutex std::mutex or something? – Manoj R Dec 28 '12 at 11:36
  • yes the mutex and the thread are both c++11 std – Jonas Eschmann Dec 28 '12 at 11:38
  • ok, maybe i am stupid, but now (i crafted a little code) that works, i just have to figure out, why it does not work in context of my real program – Jonas Eschmann Dec 28 '12 at 11:50
  • 2
    It probably won't help with your problem, but should consider using RAII wrappers (`lock_guard` or `unique_lock`) to lock the mutex, rather than locking and unlocking it by hand. That way, it won't be left locked forever if the block exits early or throws an exception. – Mike Seymour Dec 28 '12 at 11:57
  • ok, i will try using them, hopefully they are portable, because i am coding and debugging on Mac OS and want to deploy on Linux, thanks for your answer! – Jonas Eschmann Dec 28 '12 at 12:10
  • 3
    @sh4kesbeer: They're standard C++11 classes, so they should be portable to anywhere you can use `std::mutex` itself. – Mike Seymour Dec 28 '12 at 12:24
  • ok, so I made some progress, but it got more weird... When I create a ConnectionHandler and call addNewSocket at (?) it, it depends on where i put the creation and call, so when i do it straight after the execution started, everything will work, but when I put it into the context, when a client connected it will not work... – Jonas Eschmann Dec 28 '12 at 12:49
  • and this happens though neither the creation nor the function call take parameters, so that the should be context independent, or? – Jonas Eschmann Dec 28 '12 at 12:52
  • One of the few times I've seen `::std::endl` used when it really needed to be. – Omnifarious Dec 28 '12 at 13:10
  • "Assertion failed: (ec == 0)" -- this can only happen in two cases. 1. the mutex has been destructed. 2. memory has been corrupted. I confirmed this by looking at the public src for mutex.cpp (which just invokes pthread_mutex_unlock and asserts on the result) – jstine Dec 28 '12 at 21:34
  • @jstine: Yes, given the fact that it creates its own detached thread that it has no way of shutting down, it's a near certainty that this object (and the associated mutex) will be destroyed before the thread exits, which is why I gave the answer I did. – Omnifarious Dec 29 '12 at 00:24

1 Answers1

6

My guess is that your ConnectionHandler object is being destroyed somewhere. Also, you have defined ConnectionHandler::start in a silly way.

First, ConnectionHandler::start should be defined this way:

void ConnectionHandler::start(ConnectionHandler * pThis){
    pThis->main();
}

The C++11 ::std::thread class is perfectly capable of preserving the type of the function argument so there is no need to resort to void *.

Secondly, add in this code:

void ConnectionHandler::~ConnectionHandler(){
    const void * const meptr = this;
    this->connectionList_mutex.lock();
    ::std::cout << "ConnectionHandler being destroyed at " << meptr << ::std::endl;
    this->connectionList_mutex.unlock();
}

And change the constructor to read:

ConnectionHandler::ConnectionHandler(){
    const void * const meptr = this;
    ::std::cout << "ConnectionHandler being created at " << meptr << ::std::endl;
    std::thread t(&this->start, this);
    t.detach();
}

This will show you when the ConnectionHandler object is being destroyed. And my guess is that your code is destroying it while your detached thread is still running.

The meptr thing is because operator << has an overload for void * that prints out the pointer value. Printing out the pointer value for this will allow you to match up calls to the constructor and destructor if you're creating multiple ConnectionHandler objects.

Edit: Since it turned out I was correct, here is how I would recommend you write the play ConnectionHandler class:

#include <iostream>
#include <atomic>
#include <thread>
#include <chrono>
#include <mutex>

class ConnectionHandler {
 public:
   ConnectionHandler();
   ~ConnectionHandler();
   ConnectionHandler(const ConnectionHandler &) = delete;
   const ConnectionHandler &operator =(const ConnectionHandler &) = delete;

   int addNewSocket();

 private:
   int main();
   static void start(ConnectionHandler * pThis);

   ::std::mutex connectionList_mutex;
   volatile ::std::atomic_bool thread_shutdown;
   ::std::thread thread;
};

ConnectionHandler::ConnectionHandler()
     : thread_shutdown(false), thread(&this->start, this)
{
}

ConnectionHandler::~ConnectionHandler()
{
   thread_shutdown.store(true);
   thread.join();
}

void ConnectionHandler::start(ConnectionHandler * pThis){
   pThis->main();
}

int ConnectionHandler::addNewSocket(){
   ::std::lock_guard< ::std::mutex> lock(connectionList_mutex);
   ::std::cout << "test1" << ::std::endl;

   return 0;
}

int ConnectionHandler::main(){
   while(!thread_shutdown.load()){
      ::std::lock_guard< ::std::mutex> lock(connectionList_mutex);
      ::std::cout << "test2" << ::std::endl;
      ::std::this_thread::sleep_for(::std::chrono::milliseconds(100));

   }
   return 0;
}
Omnifarious
  • 54,333
  • 19
  • 131
  • 194
  • 1
    First: Thanks a lot, you guys here at stackoverflow are great! Hopefully someday i will have enough skill to help people, like you do. Second: Now that i know that the ConnectionHandler is destroyed right after it is created i am starting to investigate this, without your kind hint i would probably have just given up this project, because i have been trying to fix the error for a week now :) – Jonas Eschmann Dec 30 '12 at 12:20
  • Ok, i found the fault: I have created the ConnectionHandler within a function and as soon as the function ended the instance has been destroyed. In connection to this[link](http://stackoverflow.com/questions/6403055/object-destruction-in-c) i have figured out, that i have to create a pointer that points to a handler created with the "new" operator (ConnectionHandler* newHandler = new ConnectionHandler;). In this case (as i unsterstood) the pointer is destroyed, but the instance of the ConnectionHandler stays in memory. Again: Thanks for your kind help! – Jonas Eschmann Dec 30 '12 at 12:37
  • @sh4kesbeer: That is still likely the wrong solution. You need to think about which thread 'owns' the ConnectionManager object. In this case, the only real choice is the detached thread since that thread needs the object as long as it lives and you have no way of shutting it down. This means you need to have a way to 'ask' the detached thread to destroy your object and shut itself down. Creating it with `new` sort of works, but it creates a memory leak and is overall a messy solution. – Omnifarious Dec 30 '12 at 22:55
  • I do not understand what you mean, in context of the real project (not this minimal solution) the ConnectionHandler is created and owned by an Acceptor (Object) that processes incoming connections and disconnects. The plan (for now) is that this Acceptor will destroy and create the ConnectionHandlers dynamically as they are needed. I hope this works, concerning that the data can be accessed process wide and is not limited by threads. – Jonas Eschmann Dec 31 '12 at 17:39
  • @sh4kesbeer: If the `ConnectionHandler` is still creating a detached thread, you still need a way of notifying the detached thread that it should shut down and then `join` with it in the `ConnectionHandler` destructor. – Omnifarious Jan 01 '13 at 18:07
  • @sh4kesbeer: I redid your test/experimental `ConnectionHandler` class to show you what I was talking about. Note that my use of `::std::atomic_bool` was intentional and absolutely necessary for things to work properly. – Omnifarious Jan 01 '13 at 20:40
  • Okay, i undestand the most of it, but what do this two lines do: ConnectionHandler(const ConnectionHandler &) = delete; const ConnectionHandler &operator =(const ConnectionHandler &) = delete; ? – Jonas Eschmann Jan 02 '13 at 10:24
  • I have to add sth. here: The ConnectionHandler is not destroyed by another thread, but just out of the scope of the parent Acceptor(the ConnectionHandler thread processes the ConnectionHandlers main() and when noticing that there are no connections left it calls a function on the Acceptor, in which the ConnectionHandler is destroyed) – Jonas Eschmann Jan 02 '13 at 10:41
  • @sh4kesbeer: Those are to disable the default copy constructor and default assignment operator. Basically I'm saying that `ConnectionHandler` cannot be copied. And I don't understand your second comment. – Omnifarious Jan 02 '13 at 13:08
  • Okay, i will try to explain it better :). I have an Acceptor with a main function and a ConnectionHandler with a main function. Each of them has a own thread that runs through its main function. When a client connects (and there is no ConnectionHandler) the Acceptor will create a ConnectionHandler and pass the connection. Now the ConnectionHandler cares about the connection. As soon as it notices that the client has disconnected, the main loop of the ConnectionHandler will be interrupted and the ConnectionHandler-thread will notify the Acceptor (by a member function of the Acceptor with – Jonas Eschmann Jan 02 '13 at 14:31
  • a pointer to itself(the ConnectionHandler) as a parameter) so the thread of the ConnectionHandler jumps into the scope of the Acceptor and from there the ConnectionHandler is destroyed. – Jonas Eschmann Jan 02 '13 at 14:32
  • @sh4kesbeer: Ahh, that's a tricky situation. The original thread is still running you realize. And as soon as that `Acceptor` function is finished it will return right back to the original `ConnectionHandler` function that now has an invalid `this` pointer. If you adopt a solution like the one I suggested, you'll have a deadlock as the `ConnectionHandler` destructor waits for the `ConnectionHandler::main` to notice that it's being destroyed and end the thread, the thread the destructor is being called from and is paused. – Omnifarious Jan 02 '13 at 14:53
  • @sh4kesbeer: When doing something like that you need to think through the ways threads interact really carefully. – Omnifarious Jan 02 '13 at 14:54