C++, pthreads: how to stop a worker thread from multiple threads

Question

I need to be able to stop a single worker thread from continuing to execute from arbitrary points in arbitrary other threads, including, but not limited to, the main thread. I had produced what I thought was working code last year, but investigations to-day following some thread deadlocks showed that it does not seem to work properly, especially as regards mutexes.

The code needs to run a particular method, path_explorer_t::step(), in a worker thread exactly once for every time that a helper method, start_path_explorer() is called in the main thread. start_path_explorer() is only ever called from the main thread.

Another method, stop_path_explorer() must be able to be called at any time by any thread (other than the thread that runs path_explorer_t::step()), and must not return until it is certain that path_explorer_t::step() has fully completed.

Additionally, path_explorer_t::step() must not be called if karte_t::world->is_terminating_threads() is true, but must instead terminate the thread at the next opportunity. The thread must not terminate in other circumstances.

The code that I have written to do this is as follows:

    void* path_explorer_threaded(void* args)
    {
        karte_t* world = (karte_t*)args;
        path_explorer_t::allow_path_explorer_on_this_thread = true;
        karte_t::path_explorer_step_progress = 2;

        do
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
            karte_t::path_explorer_step_progress = 0;
            simthread_barrier_wait(&start_path_explorer_barrier);
            pthread_mutex_lock(&path_explorer_mutex);

            if (karte_t::world->is_terminating_threads())
            {
                karte_t::path_explorer_step_progress = 2;
                pthread_mutex_unlock(&path_explorer_mutex);
                break;
            }

            path_explorer_t::step();

            karte_t::path_explorer_step_progress = 1;

            pthread_cond_signal(&path_explorer_conditional_end);
            karte_t::path_explorer_step_progress = 2;
            pthread_mutex_unlock(&path_explorer_mutex);
        } while (!karte_t::world->is_terminating_threads());

        karte_t::path_explorer_step_progress = -1;
        pthread_exit(NULL);
        return args;
    }


    void karte_t::stop_path_explorer()
    {
    #ifdef MULTI_THREAD_PATH_EXPLORER
        pthread_mutex_lock(&path_explorer_mutex);

        if (path_explorer_step_progress = 0)
        {
            pthread_cond_wait(&path_explorer_conditional_end, &path_explorer_mutex);
        }

        pthread_mutex_unlock(&path_explorer_mutex);
    #endif
    }

    void karte_t::start_path_explorer()
    {
    #ifdef MULTI_THREAD_PATH_EXPLORER
        if (path_explorer_step_progress == -1)
        {
            // The threaded path explorer has been terminated, so do not wait
            // or else we will get a thread deadlock.
            return;
        }
        pthread_mutex_lock(&path_explorer_mutex);
        if (path_explorer_step_progress > 0)
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
        }
        if(path_explorer_step_progress > -1)
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
        }
        pthread_mutex_unlock(&path_explorer_mutex);
    #endif 
    }

However, I find that, for reasons that I do not understand, the mutex lock in stop_path_explorer() does not work properly, and it does not prevent the mutex lock line from being passed in path_explorer_threaded, with the consequence that it is possible for the thread calling stop_path_explorer() to be waiting at the cond_wait and the worker thread itself to be waiting at the top barrier underneath "do". It also seems to be able to produce conditions in which the mutex can be unlocked twice, which gives rise to undefined behaviour unless I set it to recursive.

Do I just need to set the mutex attribute to recursive and add an extra unlock inside the conditional statement in stop_path_explorer(), or is a more fundamental redesign needed? If the latter, has anyone any suggestions as to how to go about it?

Thank you in advance for any help.

Answer 1

Having investigated this further, I think that I have a potential answer to my own question.

I had misunderstood how pthread_cond_wait() works in conjunction with the mutex - the documentation says that it locks, not unlocks the mutex passed to it.

This means that the mutex was getting double locked from the same thread, which created undefined behaviour, and may well have resulted in some of the odd problems that I was seeing.

I have now rewritten the code as follows with a second mutex (new definitions not shown in the code sample):

    void* path_explorer_threaded(void* args)
    {
        karte_t* world = (karte_t*)args;
        path_explorer_t::allow_path_explorer_on_this_thread = true;
        karte_t::path_explorer_step_progress = 2;
        int mutex_error = 0;

        do
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
            karte_t::path_explorer_step_progress = 0;
            simthread_barrier_wait(&start_path_explorer_barrier);   

            if (karte_t::world->is_terminating_threads())
            {
                karte_t::path_explorer_step_progress = 2;
                break;
            }

            path_explorer_t::step();

            mutex_error = pthread_mutex_lock(&path_explorer_mutex);
            karte_t::path_explorer_step_progress = 1;
            mutex_error = pthread_mutex_unlock(&path_explorer_mutex);

            pthread_cond_signal(&path_explorer_conditional_end);

            mutex_error = pthread_mutex_lock(&path_explorer_mutex);
            karte_t::path_explorer_step_progress = 2;
            mutex_error = pthread_mutex_unlock(&path_explorer_mutex);

        } while (!karte_t::world->is_terminating_threads());

        karte_t::path_explorer_step_progress = -1;
        pthread_exit(NULL);
        return args;
    }


    void karte_t::stop_path_explorer()
    {
    #ifdef MULTI_THREAD_PATH_EXPLORER
        int mutex_error = 0;

        while (path_explorer_step_progress == 0)
        {
            mutex_error = pthread_mutex_lock(&path_explorer_mutex);
            pthread_cond_wait(&path_explorer_conditional_end, &path_explorer_cond_mutex);
            if (&path_explorer_mutex)
            {
                mutex_error = pthread_mutex_unlock(&path_explorer_mutex);
                mutex_error = pthread_mutex_unlock(&path_explorer_cond_mutex);
            }
        }

    #endif
    }

    void karte_t::start_path_explorer()
    {
    #ifdef MULTI_THREAD_PATH_EXPLORER
        if (path_explorer_step_progress == -1)
        {
            // The threaded path explorer has been terminated, so do not wait
            // or else we will get a thread deadlock.
            return;
        }
        if (path_explorer_step_progress > 0)
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
        }
        if(path_explorer_step_progress > -1)
        {
            simthread_barrier_wait(&start_path_explorer_barrier);
        }
    #endif 
    }

However, I do not believe that this code is working fully correctly. The software from which this is taken, an open source computer game, is designed to be playable over the internet in a multi-player configuration using lockstep networking (meaning that the server and client must execute the code from the defined start point exactly deterministically or they will get out of sync). When using this code, the clients will eventually go out of sync with the server, whereas they would not with the original code (provided, that is, that server and client were running identical executables: I was having trouble with client and server going out of sync when the executables were differently compiled, e.g. GCC and Visual Studio, and I suspect that the undefined behaviour might be the culprit there).

If anyone can confirm whether my new code is correct or has any noticeable flaws, I should be very grateful.