1

As std::vector isn't thread-safe, I was trying to build a very simple encapsulation around it which makes it thread-safe.

This works quite well, but there's one little problem. When the instance of the class is being destructed and another thread is still trying to read data out of it, the thread keeps hanging forever in the boost::mutex::scoped_lock lock(m_mutex);

How could I solve this? The best is to just unlock the mutex in order that the thread hanging in it can continue executing. I haven't defined a destructor because until now, it was not required.

Here my code. Note that there are more methods than shown here, it's been simplified.

template<class T>
class SafeVector 
{
    public:
    SafeVector();
    SafeVector(const SafeVector<T>& other);

    unsigned int size() const;
    bool empty() const;

    void clear();
    T& operator[] (const unsigned int& n);

    T& front();
    T& back();

    void push_back(const T& val);
    T pop_back();

    void erase(int i);

    typename std::vector<T>::const_iterator begin() const;
    typename std::vector<T>::const_iterator end() const;

    const SafeVector<T>& operator= (const SafeVector<T>& other);

    protected:
    mutable boost::mutex m_mutex;
    std::vector<T>  m_vector;

};

template<class T>
SafeVector<T>::SafeVector()
{

}

template<class T>
SafeVector<T>::SafeVector(const SafeVector<T>& other)
{
    this->m_vector = other.m_vector;
}

template<class T>
unsigned int SafeVector<T>::size() const
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.size();
}

template<class T>
bool SafeVector<T>::empty() const
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.empty();
}

template<class T>
void SafeVector<T>::clear()
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.clear();
}

template<class T>
T& SafeVector<T>::operator[] (const unsigned int& n)
{
    boost::mutex::scoped_lock lock(m_mutex);
    return (this->m_vector)[n];
}

template<class T>
T& SafeVector<T>::front()
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.front();
}

template<class T>
T& SafeVector<T>::back()
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.back();
}

template<class T>
void SafeVector<T>::push_back(const T& val)
{
    boost::mutex::scoped_lock lock(m_mutex);
    return this->m_vector.push_back(val);
}

template<class T>
T SafeVector<T>::pop_back()
{
    boost::mutex::scoped_lock lock(m_mutex);
    T back = m_vector.back();
    m_vector.pop_back();
    return back;
}

template<class T>
void SafeVector<T>::erase(int i)
{
    boost::mutex::scoped_lock lock(m_mutex);
    this->m_vector.erase(m_vector.begin() + i);
}

template<class T>
typename std::vector<T>::const_iterator SafeVector<T>::begin() const
{
    return m_vector.begin();
}

template<class T>
typename std::vector<T>::const_iterator SafeVector<T>::end() const
{
    return m_vector.end();
}

Edit I have to change my definition. The container clearly is not thread-safe as stated before. It isn't supposed to do so - even if the nomenclature is misleading. Sure you can do things with it that aren't thread-safe at all! But only one thread writes into the container, 2 or 3 read from it. It works well until I try to stop the process. I have to state that a monitor would have been better. But time's running out and I cannot change this until then.

Any idea is appreciated! Thanks and regards.

Atmocreations
  • 9,923
  • 15
  • 67
  • 102
  • 3
    I think you have threading issues at a higher level. It seems like an error for thread 1 to think that it is legitimate to destroy an object that thread 2 has a reference to and believes it can read from it. What if thread 1 actually managed to completely destroy the object and _then_ thread 2 tried to read from it? – CB Bailey Jan 14 '12 at 13:47
  • 1
    Wouldn't the copy constructor also have to lock the *other* vector? – Kerrek SB Jan 14 '12 at 13:53
  • I'd be interested to see the implementation of the copy assignment operator, too. It would be easy to get it spectacularly wrong. – CB Bailey Jan 14 '12 at 13:55
  • 2
    I don't see what's "safe" about this container. You freely hand out iterators references to elements, while also allowing mutating operations on the container itself - since you guard those, you clearly expect multiple threads to mutate the containers concurrently, which invalidates all iterators and references. Take a pick: either make a fully concurrent container that only allows element access by *copy*, or just be careful how you use the container. – Kerrek SB Jan 14 '12 at 13:58
  • This is not what thread safety is. `std::vector` isn't thread safe for a reason. Thread safe containers are usually immutable. Moreover, even for thread safe objects (e.g. std::atomic and boost::mutex itself) construction and destruction aren't thread safe. – Yakov Galka Jan 14 '12 at 14:08
  • There's ever only one thread modifying the container and it's content. The other threads only read the data out of it. And while an iterator is being used over the data, the data is never changed by anyone. Does anyone have an idea to release the lock from a thread which is not holding it? – Atmocreations Jan 14 '12 at 14:10
  • @Atmocreations: No, there's no way to release a mutex lock from a thread other than the one holding it. What would happen to the execution of the thread which held the lock? If you showed a complete compilable example of how you are trying to _use_ your vector then it would be possible to give you some help on the best way to solve your actual problem. – CB Bailey Jan 14 '12 at 14:16
  • @ybungalobill That's clearly an overstatement of what thread-safe classes are. The general model of mutable thread-safe data structures is that of a [monitor](http://en.wikipedia.org/wiki/Monitor_%28synchronization%29) (which predates invention of threads by a couple of decades). The class at hand is indeed a major thread issue! – Dietmar Kühl Jan 14 '12 at 14:28
  • @Atmocreations You should not have **any** thread holding a lock for an extensive time unless you can accept that only this thread makes any progress. From the sounds of it, you might want to look at _condition variables_ because this is the way to notify another thread that it is time to proceed in some way. – Dietmar Kühl Jan 14 '12 at 14:32
  • @DietmarKühl: You completely misread by comment. I had *two* statements: 1) about immutability: "thread safe *containers* are *usually* immutable", and 2) about thread-safe classes in general: "even for thread safe objects..." – Yakov Galka Jan 14 '12 at 14:32
  • @DietmarKühl: Right, I know that. And I'm not holding the lock for a long time. Operations on the container don't take a long time. I have an object `A` storing some "SafeVectors". A thread (not the same that created object `A`) changes the data in these SafeVectors during a loop. Another one reads from them. Now when I want to shut down the whole process, it seems that one thread always hangs in the lock, actually preventing `A` from being destroyed. – Atmocreations Jan 14 '12 at 14:45
  • @Atmocreations Well, before you can start destroying your thread-unsafe "SafeVector" you need to make sure nobody is trying to use it. That is, whoever starts to destroy the object needs to know that no other thread is doing to access this data structure. A corresponding indication, e.g. a reference count, could be stored inside a class but the control of this object being shut down is something you need to have in each class: when the destruction started, you can't have it accessed. – Dietmar Kühl Jan 14 '12 at 14:59
  • Depending on your environment, you might want to try ``, which is available in Visual Studio since 2010 IIRC. – Xeo Jan 15 '12 at 04:26
  • I'm using gcc on linux. So there's no Visual Studio. But thanks anyway. – Atmocreations Jan 15 '12 at 09:32

1 Answers1

1

EDIT: Updated to be more complete example.

Others have pointed out the flaws with your "thread safety;" I'll attempt to answer your question.

The only proper way to do what you have set out to do is to make sure all of your threads have been shutdown before you try and destroy the vector itself.

A common method I have used is to simply use RAII to define the order of construction and destruction.

void doSomethingWithVector(SafeVector &t_vec)
{
  while (!boost::this_thread::interruption_requested())
  {
    //operate on t_vec
  }
}

class MyClassThatUsesThreadsAndStuff
{
  public:
    MyClassThatUsesThreadsAndStuff()
      : m_thread1(&doSomethingWithVector, boost::ref(m_vector)),
        m_thread2(&doSomethingWithVector, boost::ref(m_vector))
    {
      // RAII guarantees that the vector is created before the threads
    }

    ~MyClassThatUsesThreadsAndStuff()
    {
      m_thread1.interrupt();
      m_thread2.interrupt();
      m_thread1.join();
      m_thread2.join();
      // RAII guarantees that vector is freed after the threads are freed
    }

  private:
    SafeVector m_vector;
    boost::thread m_thread1;
    boost::thread m_thread2;
};

If you are looking for a more complete thread safe data structure that allows for multiple readers and writers, feel free to check out a queue I wrote using boost threads a while back.

http://code.google.com/p/crategameengine/source/browse/trunk/include/mvc/queue.hpp

lefticus
  • 3,346
  • 2
  • 24
  • 28
  • thanks for your response. i know about RAII. the "problem" here is that the joins don't always return... if I overlook your queue implementation, the principle seems to be the same to me: putting a scoped_lock everywhere you perform operations on the container - except that my implementation is lacking of the cancellation part. – Atmocreations Jan 15 '12 at 09:35
  • I have blocking reads in my Queue class, which is why the cancellation is required. You do not have any "block and wait for data" features of your class. This lead me to assume the actual error is that the Mutex you were trying to access had been destroyed before / during a lock. The only way this could happen is if the Vector were destroyed *before* the thread. With my example above, you have now a cancellation example as well as guarantees that the the mutex cannot be destroyed before its useful life is done. – lefticus Jan 15 '12 at 15:21