Returning an iterator in a multi threaded environment, a good idea?

Question

Is it a good idea to return an iterator on a list in an object that is used and shared in a multi threaded environment?

class RequestList
{
public:
    RequestList::RequestList();
    RequestList::~RequestList();

    std::list<boost::shared_ptr<Request> >::iterator GetIterator();
    int ListSize();
    void AddItem(boost::shared_ptr<Request> request);
    void RemoveItem(boost::shared_ptr<Request> request);
    std::list<boost::shared_ptr<Request> > GetRequestsList();
    boost::shared_ptr<Request> GetRequest();

private:
    std::list<boost::shared_ptr<Request> > requests;
    std::list<boost::shared_ptr<Request> >::iterator iter;   //Iterator
    boost::mutex listmtx;
};




std::list<boost::shared_ptr<Request> >::iterator RequestList::GetIterator()
{
    return this->iter;
}

USE:

RequestList* requests;

In some thread (may be used again in other threads)

 std::list<boost::shared_ptr<Request> >::iterator iter = requests->GetIterator();

Or would it be smarter to just create an iterator for that list each time and use it locally within each thread?

Answer 1

Depends how the list is used, but from what you've shown it looks wrong. The iterator becomes invalid if the element it refers to is removed: the element in this case being the shared_ptr object in the list.

As soon as you release the mutex, I guess some other thread could come along and remove that element. You haven't shown code that does it, but if it can happen, then iterators shouldn't "escape" the mutex.

I assume this is a "self-synchronizing" container, since the mutex is private and there's nothing in the API to lock it. The fundamental difficulty with such things, is that it's not thread-safe to perform any kind of iteration on them from the outside. It's easy enough to provide a thread-safe queue, that supports:

• adding an element,
• removing an element by value,
• removing the head and returning a copy of its value.

Beyond that, it's harder to provide useful basic operations, because almost anything that manipulates the list in any interesting way needs to be done entirely under the lock.

By the looks of things, you can copy the list with GetRequestsList, and iterate over the copy. Not sure whether it will do you any good, since the copy is instantly out of date.

Answer 2

No it is usually not a good idea to share an iterator across threads. There are a couple of ways to make sure you don't get in trouble.

First off, an iterator is a light-weight object which is fast to construct and takes up very little memory. So you should not be concerned about any performance-issues. Just create an instance whenever you need one.

That said you do have to make sure that your list is not altered when you are iterating. I see you have a boost::mutex on in your class. Locking that will be perfect for ensuring that you don't get any problems when iterating.

A different and equally valid way of handling these situations is to simply copy the internal list and iterate that. This is a good solution if you require that the list is continually updated and you don't want other threads waiting. Of course it takes up a bit more memory, but since you are storing smart pointers, it will hardly be anything at all.

Answer 3

Accessing the list via iterators in multiple threads where the main list itself is not locked is dangerous.

There's no guarantee what state the list will be as you do things in with the iterators in different threads (for example, one thread could happily iterate through and erase all the items, what will the other thread - who's also iterating, see?)

If you are going to work on the list in multiple threads, lock the whole list first, then do what you need to. Copying the list is an option, but not optimal (depending on the size of your list and how fast it's updated). If locking becomes a bottle neck, re-think your architecture (list per thread for example?)

Answer 4

Each thread that calls the GetIterator function will get its own copy of the stored iterator in the list. As a std::list<>::iterator is a bi-directional iterator, any copies you make are completely independent of the source. If one of them changes, this will not be reflected in any of the other iterators.

As for using iterator in a multi-threaded environment, this is not that different from a single-threaded environment. The iterator remains valid as long as the element it refers to is part of the container. You just have to take care of proper synchronization when accessing/modifying the container or its elements.

Answer 5

If the list is modified by one of your threads you might get into trouble.

But of course, you can take care of that by setting locks and ro- and rw-locks during modification. But since mutexes are the scurge of any high performance program, maybe you can make a copy of the list (or references) and keep the original list mutex- and lock-free? That would be the best way.

If you have the mutexes in place you only have to battle with the issues of a modifying a list while holding iterators on it as you would normally do anyway -- i.e. adding elements should be ok, deletion should be done "careful" but doing it on a list is probably less likely to explode as on a vector:-)

Answer 6

I would reconsider this design and would use a task-based approach. This way you don't need any mutexes. For example use Intel TBB, which initializes a task pool internally. So you can easily implement a one-writer/multiple-readers concept. First add all requests to your request container (a simple std::vector might be better suited in terms of cache locality and performance) and then do a parallel_for() over you request-vector BUT DON'T remove a request in your parallel-for() functor!
After the processing you can actually clear your request vector without any need of locking a mutex. That's it!
I hope I could help a bit.