1

I have a standard vector and multiple threads. I'm using the following code to lock when it's required:

boost::mutex::scoped_lock lock(mutex);

This works properly, the application runs without any problems, but now I created a small class for the vector to make my life easier:

template <class T> class FVector
{
private:
    std::vector<T>          standard_vector;
    mutable boost::mutex    mutex;

public:
    typedef typename std::vector<T>::iterator  iterator;
    typedef typename std::vector<T>::size_type size_type;

    FVector(void)
    {

    }

    iterator begin(void)
    {
        boost::mutex::scoped_lock lock(mutex);
        return standard_vector.begin();
    }

    iterator end(void)
    {
        boost::mutex::scoped_lock lock(mutex);
        return standard_vector.end();
    }

    void push_back(T & item)
    {
        boost::mutex::scoped_lock lock(mutex);
        standard_vector.push_back(item);
    }

    void erase(iterator it)
    {
        boost::mutex::scoped_lock lock(mutex);
        standard_vector.erase(it);
    }
};

But unfortunately it doesn't work. I'm simply getting xx.exe has triggered a breakpoint. exception, which means there is something wrong with the locks and multiple threads trying to write and read in the same time.

I'm using the following code for testing:

#include <Windows.h>
#include <process.h>
#include "thread_safe_vector.h"

struct TValue
{
    int value;
};

FVector<TValue> vec_Safe;

boost::mutex testMutex;

void thread2(void* pArg)
{
    while (true)
    {
        //boost::mutex::scoped_lock lock(testMutex);

        for (FVector<TValue>::iterator it = vec_Safe.begin(); it != vec_Safe.end(); it++)
        {
            if (it->value == 5)
            {
                vec_Safe.erase(it);
                break;
            }
        }
    }
}

void thread1(void* pArg)
{
    while (true)
    {
        TValue value;
        value.value = 5;

        //boost::mutex::scoped_lock lock(testMutex);

        vec_Safe.push_back(value);
    }
}

void main(void)
{
    HANDLE hThreads[50];

    for (size_t i = 0; i < 50; i++)
    {
        hThreads[i] = (HANDLE)_beginthread(i % 2 == 0 ? thread1 : thread2, NULL, NULL);
    }

    system("pause");

    for (size_t i = 0; i < 50; i++)
    {
        TerminateThread(hThreads[i], 0);
    }
}

I'm absolutely out of ideas, I tried to figure out the problem for hours... Is there anything what I'm doing wrong?

CsOkemf
  • 175
  • 1
  • 1
  • 7

1 Answers1

3

By moving the locks down to the lower abstraction level you introduced data races.

    for (FVector<TValue>::iterator it = vec_Safe.begin(); it != vec_Safe.end(); it++)

Here both begin() and end() execute under a lock, but the comparison doesn't. Much worse:

    {
        if (it->value == 5)

dereferences a stale iterator it some other party modified it.

Even more fundamentally, any reallocating operation can invalidate all iterators in one fell swoop.

You will always have to lock for the entire loop. If loop body takes signifcant time, you could copy the elements under the lock, and process them afterwards.

However in such cases it's more typical to use a (lockfree) queue and splice the elements from the shared queue to a local one for more efficiency.

Community
  • 1
  • 1
sehe
  • 374,641
  • 47
  • 450
  • 633
  • Thank you for your fast answer, so it looks there is no way to do the locks just in the "FVector" class? I wanted to prevent myself from adding the locks to each loops. – CsOkemf Mar 15 '15 at 00:04
  • @CsOkemf not the way you thought. You can design APIs that return adoptable locks from their operations, but such designs are not often much clearer in my experience. – sehe Mar 15 '15 at 00:13
  • Hmm, thank you. Maybe there is something what you recommend to use? – CsOkemf Mar 15 '15 at 00:16
  • @CsOkemf maybe there is, but is will largely depend on the application requirements. I do not suggest posting such a question here. [CodeReview.SE] or [Programmers.SE] could be more hospitable – sehe Mar 15 '15 at 00:17