How do I use std::lock_guard to lock read and write access to a std::map?

Question

When I step through the following code in MSVC 2012 I place a breakpoint on line 3. and line 8.

First break comes on line 8.

The lock_guard is called just fine and then we break on line 3. This time, since a lock is already obtained, an exception is thrown when I step over.

I would really like it to just move on since it's still the same thread calling (we just came from line 11.)

Is there another locking mechanism better suited for this scenario?

I have a background in native win32 programming so I'm used to WaitForSingleObject which would just let me through here without fuss, but lock_guard doesn't.

Am I supposed to handle the exception? None of the examples I've seen have any kind of exception handler for lock_guard...

Is there a better way to make sure the map isnt accessed by more than one thread at a time? I need both write and read lock on it, and lock_guard seemed like a smooth alternative since I dont have to ReleaseMutex...

    //Cars.h
    mutable std::mutex carsMutex;
    class Cars
    {
    private:
        std::map<std::string,Cars> _cars;
    public:
        virtual ~Cars() {}
        Cars() {}
        Cars & operator[](const std::string &key);
        Cars & operator[](const DWORD &key);
        std::string color;
    };

    //Cars.cpp
      #include "Cars.h"
1.    Cars & Cars::operator[](const std::string &key)
2.    {
3.        std::lock_guard<std::mutex> lock_a(carsMutex);
4.        return _cars[key];
5.    }
6.    Cars & Cars::operator[](const DWORD &key)
7.    {
8.        std::lock_guard<std::mutex> lock_a(carsMutex);
9.        std::stringstream ss;
10.       ss << key;
11.       return operator[](ss.str());
12.   }
14.   void main()
15.   {
16.       //ok i have multiple threads like this one writing and reading from the map
17.       Cars cars;
18.       cars[(DWORD)2012]["volvo"].color = "blue";
19.   }

UPDATE: Here is my edit of the code above. I have taken the answer into account and this is my new attempt to correctly use std::lock_guard Please comment if its not correct.

    //Cars.h
    mutable std::recursive_mutex carsMutex;
    class Cars
    {
    private:
        std::string _color;
        std::map<std::string,Cars> _cars;
    public:
        virtual ~Cars() {}
        Cars() {}
        Cars & operator[](const std::string &key);
        Cars & operator[](const DWORD &key);
        void color(const std::string &color);
        std::string color();
    };

       //Cars.cpp
       #include "Cars.h"
 1.    Cars & Cars::operator[](const std::string &key)
 2.    {
 3.        std::lock_guard<std::recursive_mutex> lock(carsMutex);
 4.        return _cars[key];
 5.    }
 6.    Cars & Cars::operator[](const DWORD &key)
 7.    {
 8.        std::lock_guard<std::recursive_mutex> lock(carsMutex);
 9.        std::stringstream ss;
10.        ss << key;
11.        return operator[](ss.str());
12.    }
13.    void color(const std::string &color)
14.    {
15.        std::lock_guard<std::recursive_mutex> lock(carsMutex);
16.        _color = color;
17.    }
18.    std::string color()
19.    {
20.        std::lock_guard<std::recursive_mutex> lock(carsMutex);
21.        return _color;
22.    }
23.
24.    Cars cars;//this is global...
25.    void main()
26.    {
27.        //ok i have multiple threads like this one writing and reading from the map
28.        cars[(DWORD)2012]["volvo"].color("blue");
29.    }

Mike Seymour · Accepted Answer · 2012-08-23T22:41:04.073

4

To allow a thread to reacquire a mutex that it already has, you need std::recursive_mutex rather than std::mutex.

However, you have a bigger problem: your accessors are unlocking the mutex before returning the reference that you use to assign the map element, so the assignment itself is unguarded. This can be fixed by either writing a "setter" function that does the assignment before returning; or by returning a proxy object that contains a mutex lock that's released in its destructor. Solving this problem will probably remove the need for recursive_mutex.

edited Aug 23 '12 at 22:41

answered Aug 23 '12 at 22:35

Mike Seymour

249,747
28
448
644

ok that sounded promising, but then I get this error : error C2664: 'std::lock_guard<_Mutex>::lock_guard(_Mutex &)' : cannot convert parameter 1 from 'std::recursive_mutex' to 'std::mutex &' Should I just cast it? – user1621065 Aug 23 '12 at 22:41
1

@user1621065: Have you changed the lock guards to `lock_guard`? And have you read my updated answer, which explains the deeper problem behind your perceived need for a recursive mutex? – Mike Seymour Aug 23 '12 at 22:42
ok that was it :) thanks, so simple and yet I couldnt figure it out, besides from this, if another thread tries to obtain the lock while its locked will it throw an exception or just wait until lock is open and then enter? – user1621065 Aug 23 '12 at 22:47
ok thats exactly what i want, other than that, does the code look ok or is there anything else im missing? – user1621065 Aug 23 '12 at 22:49
@user1621065: The code is horribly wrong, for the reason I described in the second paragraph of the answer. You need to lock the mutex around the whole assignment operation, not just getting the reference to the element you're assigning to. Once you've fixed that, you almost certainly won't need a recursive mutex at all. – Mike Seymour Aug 23 '12 at 22:50
I was under the impression that the destructor for lock_a is called before the return... – user1621065 Aug 23 '12 at 22:54
@user1621065: Yes it is. And then you write to the map *after* the mutex has been unlocked, giving a data race. You need to keep the mutex locked for the whole operation, and also for any operations that read from the map. – Mike Seymour Aug 23 '12 at 22:56
I dont want to offend you since your answer clearly helped me move on, im just trying to learn. I read in another post that "The return value is constructed before the local variables are destroyed, and thus before the lock is released." (http://stackoverflow.com/questions/3856729/how-to-use-lock-guard-when-returning-protected-data) – user1621065 Aug 23 '12 at 23:06
2

@user1621065: Yes, but you're returning a reference, and then writing to that reference after the lock is released, while some other thread could also be trying to access the same map element. You need to keep the mutex locked until the whole operation is complete. – Mike Seymour Aug 23 '12 at 23:18
I think I understand what you mean now, so if I copy line 8 to line 16 as well, and as long as I lock_guard like that before any [] operations its threadsafe? I wish I could write a setter function, but that would defeat the whole purpose of directaccessing the map through the [] operator and I have many properties for the cars... not just color... – user1621065 Aug 23 '12 at 23:30
2

@user1621065: As this fundamental error shows, your locks are in the wrong place. You are locking the *map* operations and what you wanted to lock are the operations on the `Cars` in the map. So move your locks to where the operations they're supposed to protect are done. (Or, if absolutely necessary, have the `map` provide a "give me a lock holder object" function to higher-level callers. But this is inferior.) – David Schwartz Aug 24 '12 at 00:14
ok I wasnt allowed to post updated code, it was deleted by a moderator, so I dont know if im supposed to paste that in the original post or as a comment, seems like others that come here would beneifit from seing a fixed example, but I dont know what the accepted procedure for this is. If I just change the original post, then the original question looses its point as that was solved, maybe I should just open a new question? – user1621065 Aug 24 '12 at 21:47

How do I use std::lock_guard to lock read and write access to a std::map?

1 Answers1