const cast to allow read lock, does this smell bad ?

Question

I want to execute a read-only method on an object marked as const, but in order to do this thread-safely, I need to lock a readers-writer mutex:

const Value Object::list() const {
  ScopedRead lock(children_);
  ...
}

But this breaks because the compiler complains about "children_" being const and such. I went up to the ScopedRead class and up to the RWMutex class (which children_ is a sub-class) to allow read_lock on a const object, but I have to write this:

inline void read_lock() const {
  pthread_rwlock_rdlock(const_cast<pthread_rwlock_t*>(&rwlock_));
}

I have always learned that const_cast is a code smell. Any way to avoid this ?

Answer 1

Make the lock mutable.

Answer 2

Make the lock mutable

mutable pthread_rwlock_t rwlock;

This is a common scenario in which mutable is used. A read-only query of an object is (as the name implies) an operation that should not require non-const access. Mutable is considered good practice when you want to be able to modify parts of an object that aren't visible or have observable side-effects to the object. Your lock is used to ensure sequential access to the object's data, and changing it doesn't effect the data contained within the object nor have observable side-effects to later calls so it is still honoring the const-ness of the object.

Answer 3

To solve the actual problem, declare the lock as mutable.

The following is now my professional opinion:

The compiler is right to complain, and you are right to find this mildly offensive. If performing a read-only operation requires a lock, and locks must be writeable to lock, then you should probably make the read-only query require non-const access.

EDIT: Alright, I'll bite. I've seen this kind of pattern cause major perf hits in places you would not expect. Does anyone here know how tolower or toupper can become a major bottleneck if called frequently enough, even with the default ASCII locale? In one particular implementation of the C runtime library built for multithreading, there was a lock taken to query the current locale for that thread. Calling tolower on the order of 10000 times or more resulted in more of a perf hit than reading a file from disk.

Just because you want read-only access doesn't mean that you should hide the fact that you need to lock to get it.

Answer 4

Yes, use mutable. It's designed for this very purpose: Where the entire context of the function is const (i.e. an accessor or some other logically read-only action.) but where some element of writable access is needed for a mutex or reference counter etc.

The function should be const, even if it does lock a mutex internally. Doing so makes the code thread-neutral without having to expose the details, which I presume is what you're trying to do.

There are very few places where const_cast<> needs to be legitimately used and this isn't one of them. Using const cast on on an object, especially in a const function is a code maintenance nightmare. Consider:

token = strtok_r( const_cast<char*>( ref_.c_str() ), ":", &saveptr );

In fact, I'd argue that when you see const_cast in a const function, you should start by making the function non-const (very soon after you should get rid of the const_cast and make the function const again though)

Answer 5

Well, if we are not allowed to modify the declaration of the variable, then const_cast comes to the rescue. If not, making it mutable is the solution.