Should I use const to make objects thread-safe?

I wrote a class which instances may be accessed by several threads. I used a trick to remember users they have to lock the object before using it. It involves keeping only const instances. When in the need to read or modify sensitive data, other classes should call a method (which is const, thus allowed) to get a non-const version of the locked object. Actually it returns a proxy object 开发者_如何转开发containing a pointer to the non-const object and a scoped_lock, so it unlocks the object when going out of scope. The proxy object also overloads operator-> so the access to the object is transparent.

This way, shooting onself's foot by accessing unlocked objects is harder (there is always const_cast).

"Clever tricks" should be avoided, and this smells bad anyway.

Is this design really bad ? What else can I or should I do ?

Edit: Getters are non-const to enforce locking.

Basic problem: a non-const reference may exist elsewhere. If that gets written safely, it does not follow that it can be read safely -- you may look at an intermediate state.

Also, some const methods might (legitimately) modify hidden internal details in a thread-unsafe way.

Analyse what you're actually doing to the object and find an appropriate synchronisation mode.

If your clever container really does know enough about the objects to control all their synchronisation via proxies, then make those objects private inner classes.

This is clever, but unfortunately doomed to fail.

The problem, underlined by spraff, is that you protect against reads but not against writes.

Consider the following sequence:

unsigned getAverageSalary(Employee const& e) {
  return e.paid() / e.hired_duration();
}

What happens if we increment paid between the two function calls ? We get an incoherent value.

The problem is that your scheme does not explicitly enforce locking for reads.

Consider the alternative of a Proxy pattern: The object itself is a bundle of data, all privates. Only a Proxy class (friend) can read/write its data, and when initializing the Proxy it grabs the lock (on the mutex of the object) automatically.

class Data {
  friend class Proxy;
  Mutex _mutex;
  int _bar;
};

class Proxy {
public:
  Proxy(Data& data): _lock(data._mutex), _data(data) {}

  int bar() const { return _data._bar; }
  void bar(int b) { _data._bar = b; }

private:
  Proxy(Proxy const&) = delete; // disable copy

  Lock _lock;
  Data& _data;
};

If I wanted to do what you are doing, I would do one of the following.

Method 1:

shared_mutex m;  // somewhere outside the class

class A
{
private:
    int variable;
public:
    void lock() { m.lock(); }
    void unlock() { m.unlock(); }
    bool is_locked() { return m.is_locked(); }
    bool write_to_var(int newvalue)
    {
        if (!is_locked())
            return false;
        variable = newvalue;
        return true;
    }
    bool read_from_var(int *value)
    {
        if (!is_locked() || value == NULL)
            return false;
        *value = variable;
        return true;
    }
};

Method 2:

shared_mutex m;  // somewhere outside the class

class A
{
private:
    int variable;
public:
    void write_to_var(int newvalue)
    {
        m.lock();
        variable = newvalue;
        m.unlock();
    }
    int read_from_var()
    {
        m.lock();
        int to_return = variable;
        m.unlock();
        return to_return;
    }
};

The first method is more efficient (not locking-unlocking all the time), however, the program may need to keep checking the output of every read and write to see if they were successful. The second method automatically handles the locking and so the programmer wouldn't even know the lock is there.

Note: This is not code for copy-paste. It shows a concept and sketches how it's done. Please don't comment saying you forgot some error checking somewhere.

This sounds a lot like Alexandrescu's idea with volatile. You're not using the actual semantics of const, but rather exploiting the way the type system uses it. In this regard, I would prefer Alexandrescu's use of volatile: const has very definite and well understood semantics, and subverting them will definitely cause confusion for anyone reading or maintaining the code. volatile is more appropriate, as it has no well defined semantics, and in the context of most applications, is not used for anything else.

And rather than returning a classical proxy object, you should return a smart pointer. You could actually use shared_ptr for this, grabbing the lock before returning the value, and releasing it in the deleter (rather than deleting the object); I rather fear, however, that this would cause some confusion amongst the readers, and I would probably go with a custom smart pointer (probably using shared_ptr with the custom deleter in the implementation). (From your description, I suspect that this is closer to what you had in mind anyway.)