Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So after learning that stl map containers are not inherently atomic and therefore not thread-safe (check out this related stackoverflow question and usage example), I decided to create code that would allow concurrent access to the container.

Here is the code:

#ifndef MAP_GUARD_H_
#define MAP_GUARD_H_

/*
This class was designed to make the standard map synchronized for the basic functions.
The intent was for it to be filled with shared_ptr as the value in the pair.
A Grand Central Dispatch serial queue is used for syncronising access to the map rather than raw mutexes.
*/

// A macro to disallow the copy constructor and operator= functions
// This should be used in the private: declarations for a class
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
  TypeName(const TypeName&);               \
  void operator=(const TypeName&)

#include <map>
#ifdef WIN32
#include <xdispatch\dispatch.h>
#else
#include <xdispatch/dispatch.h>
#endif

template <class K, class V>
class map_guard
{
public:
    map_guard();
    ~map_guard();
    void clear();
    void erase(const K& key);
    void insert(const K& key, const V& value);
    /* Take a snopshot of map_ and fill target with it (usefull if you want to use iterators) */
    void fill_map(std::map<K, V>& target) const;
    V find(const K& key) const;
    size_t size() const;
    bool empty() const;
    void swap(std::map<K, V>& map);
private:
    void make_swap(std::map<K, V>* map);
    void get(std::map<K, V>* target) const;
    void get(const K& key, V* target) const;
    void get_empty(bool* empty) const;
    void get_size(size_t* size) const;
    std::map<K, V> map_;
    xdispatch::queue* dispatch_queue_;
    DISALLOW_COPY_AND_ASSIGN(map_guard);
};

template <class K, class V>
map_guard<K, V>::map_guard() : dispatch_queue_(0)
{
    dispatch_queue_ = new xdispatch::queue("test");
}

template <class K, class V>
map_guard<K, V>::~map_guard()
{
    if(dispatch_queue_)
    {
        delete dispatch_queue_;
        dispatch_queue_ = 0;
    }
}

template <class K, class V>
void map_guard<K, V>::clear()
{
    dispatch_queue_->sync(${
        map_.clear();
    });
}

template <class K, class V>
void map_guard<K, V>::erase(const K& key)
{
    dispatch_queue_->sync(${
        map_.erase(key);
    });
}

template <class K, class V>
void map_guard<K, V>::insert(const K& key, const V& value)
{
    dispatch_queue_->sync(${
        map_[key] = value;
    });
}

template <class K, class V>
void map_guard<K, V>::get(std::map<K, V>* target) const
{
    dispatch_queue_->sync(${
        target->insert(map_.begin(), map_.end());
    });
}

template <class K, class V>
void map_guard<K, V>::get(const K& key, V* target) const
{
    dispatch_queue_->sync(${
        std::map<K, V>::const_iterator it = map_.find(key);
        if(it != map_.end())
        {
            *target = it->second;
        }
    });
}

template <class K, class V>
void map_guard<K, V>::fill_map(std::map<K, V>& target) const
{
    get(&target);
}

template <class K, class V>
V map_guard<K, V>::find(const K& key) const
{
    V temp;
    get(key, &temp);
    return temp;
}

template <class K, class V>
void map_guard<K, V>::get_size(size_t* size) const
{
    dispatch_queue_->sync(${
        *size = map_.size();
    });
}

template <class K, class V>
size_t map_guard<K, V>::size() const
{
    size_t size;
    get_size(&size);
    return size;
}

template <class K, class V>
void map_guard<K, V>::get_empty(bool* empty) const
{
    dispatch_queue_->sync(${
        *empty = map_.empty();
    });
}

template <class K, class V>
bool map_guard<K, V>::empty() const
{
    bool empty;
    get_empty(&empty);
    return empty;
}

template <class K, class V>
void map_guard<K, V>::swap(std::map<K, V>& map)
{
    make_swap(&map);
}

template <class K, class V>
void map_guard<K, V>::make_swap(std::map<K, V>* map)
{
    dispatch_queue_->sync(${
        map_.swap(*map);
    });
}

#endif

Is this a valid and code correct method for solving the problem? Are there other ways that may be more efficient?

share|improve this question

2 Answers 2

Use boost::noncopyable instead of DISALLOW_COPY_AND_ASSIGN:

   template <class K, class V>
   class map_guard: boost::noncopyable
   {
  1. boost::noncopyable resides in the first line instead of deep internals

  2. DRY -- you do not need to type map_guard again

  3. noncopyable.hpp is a small header-only library


Probably, you do not need special case #include <xdispatch\dispatch.h> because compiler will translate #include <xdispatch/dispatch.h> for you.


Next, I have a vague idea "what an xdispatch is", but it seems to me that placing block in a queue is a costly operation, so locking have to be used. Also, may be lockless algorith will be appropriate, say, based on http://en.wikipedia.org/wiki/Compare-and-swap

share|improve this answer
    
Wouldn't #include <xdispatch/dispatch.h> work in Windows anyway? Windows supports both characters as path separators. –  seand Feb 6 '12 at 23:54
    
@seand My last compiler run under Windows was around 10 years ago. –  user1123502 Feb 7 '12 at 0:28

If you would like to try out c++0x, you have std::mutex and std::lock_guard. An example forwarding some of methods in std::map is shown as following:

//hello.cc

#include <map>
#include <mutex>
#include <iostream>

template <class K, class V, class Compare = std::less<K>, class Allocator = std::allocator<std::pair<const K, V> > >
class guarded_map {
    private:
        std::map<K, V, Compare, Allocator> _map;
        std::mutex _m;

    public:
        void set(K key, V value) {
            std::lock_guard<std::mutex> lk(this->_m);
            this->_map[key] = value;
        }

        V & get(K key) {
            std::lock_guard<std::mutex> lk(this->_m);
            return this->_map[key];
        }

        bool empty() {
            std::lock_guard<std::mutex> lk(this->_m);
            return this->_map.empty();
        }

        // other public methods you need to implement
};

int main(int argc, char ** argv) {
    guarded_map<int, int> m;
    m.set(1, 10);
    m.set(2, 20);
    m.set(4, 30);
    std::cout<<"m[2]="<<m.get(2)<<std::endl;
    return 0;
}

To compile, you will probably need to add some special flag to your compiler. For example, g++ would be:

g++ -std=c++0x -o hello hello.cc

The std::lock_guard here makes things much easier. It locks the mutex in its constructor, and automatically unlock the mutex in its destructor, which means you can initialize a std::lock_guard at the beginning of a block you need to synchronize concurrency, and leave it there, as the lock will be released when this block exit.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.