C++11 reentrant class locking strategy

Question

I have an interface using the pimpl idiom, however the interface needs to be reentrant. Calling threads do not need to be aware of the locking, however. This is four part question and one part gratuitously contrived C++11 example (example included to address several FAQ-like questions that I've run across re: locking, pimpl, rvalue and C++11 where the answers were somewhat dubious in their quality).

In the header, example.hpp:

#ifndef EXAMPLE_HPP
#define EXAMPLE_HPP

#include <memory>
#include <string>

#ifndef BOOST_THREAD_SHARED_MUTEX_HPP
# include <boost/thread/shared_mutex.hpp>
#endif

namespace stackoverflow {

class Example final {
public:
  typedef ::boost::shared_mutex shared_mtx_t;
  typedef ::boost::shared_lock< shared_mtx_t > shared_lock_t;
  typedef ::boost::unique_lock< shared_mtx_t > unique_lock_t;

  Example();
  Example(const std::string& initial_foo);

  ~Example();
  Example(const Example&) = delete;             // Prevent copying
  Example& operator=(const Example&) = delete;  // Prevent assignment

  // Example getter method that supports rvalues
  std::string foo() const;

  // Example setter method using perfect forwarding & move semantics. Anything
  // that's std::string-like will work as a parameter.
  template<typename T>
  bool foo_set(T&& new_val);

  // Begin foo_set() variants required to deal with C types (e.g. char[],
  // char*). The rest of the foo_set() methods here are *NOT* required under
  // normal circumstances.

  // Setup a specialization for const char[] that simply forwards along a
  // std::string. This is preferred over having to explicitly instantiate a
  // bunch of const char[N] templates or possibly std::decay a char[] to a
  // char* (i.e. using a std::string as a container is a Good Thing(tm)).
  //
  // Also, without this, it is required to explicitly instantiate the required
  // variants of const char[N] someplace. For example, in example.cpp:
  //
  // template bool Example::foo_set<const char(&)[6]>(char const (&)[6]);
  // template bool Example::foo_set<const char(&)[7]>(char const (&)[7]);
  // template bool Example::foo_set<const char(&)[8]>(char const (&)[8]);
  // ...
  //
  // Eww. Best to just forward to wrap new_val in a std::string and proxy
  // along the call to foo_set<std::string>().
  template<std::size_t N>
  bool foo_set(const char (&new_val)[N]) { return foo_set(std::string(new_val, N)); }

  // Inline function overloads to support null terminated char* && const
  // char* arguments. If there's a way to reduce this duplication with
  // templates, I'm all ears because I wasn't able to generate a templated
  // versions that didn't conflict with foo_set<T&&>().
  bool foo_set(char* new_val)       { return foo_set(std::string(new_val)); }
  bool foo_set(const char* new_val) { return foo_set(std::string(new_val)); }

  // End of the foo_set() overloads.

  // Example getter method for a POD data type
  bool bar(const std::size_t len, char* dst) const;
  std::size_t bar_capacity() const;

  // Example setter that uses a unique lock to access foo()
  bool bar_set(const std::size_t len, const char* src);

  // Question #1: I can't find any harm in making Impl public because the
  // definition is opaque. Making Impl public, however, greatly helps with
  // implementing Example, which does have access to Example::Impl's
  // interface. This is also preferre, IMO, over using friend.
  class Impl;

private:
  mutable shared_mtx_t rw_mtx_;
  std::unique_ptr<Impl> impl_;
};

} // namespace stackoverflow

#endif // EXAMPLE_HPP

And then in the implementation:

#include "example.hpp"

#include <algorithm>
#include <cstring>
#include <utility>

namespace stackoverflow {

class Example;
class Example::Impl;


#if !defined(_MSC_VER) || _MSC_VER > 1600
// Congratulations!, you're using a compiler that isn't broken

// Explicitly instantiate std::string variants
template bool Example::foo_set<std::string>(std::string&& src);
template bool Example::foo_set<std::string&>(std::string& src);
template bool Example::foo_set<const std::string&>(const std::string& src);

// The following isn't required because of the array Example::foo_set()
// specialization, but I'm leaving it here for reference.
//
// template bool Example::foo_set<const char(&)[7]>(char const (&)[7]);
#else
// MSVC workaround: msvc_rage_hate() isn't ever called, but use it to
// instantiate all of the required templates.
namespace {
  void msvc_rage_hate() {
    Example e;
    const std::string a_const_str("a");
    std::string a_str("b");
    e.foo_set(a_const_str);
    e.foo_set(a_str);
    e.foo_set("c");
    e.foo_set(std::string("d"));
  }
} // anon namespace
#endif // _MSC_VER



// Example Private Implementation

class Example::Impl final {
public:
  // ctors && obj boilerplate
  Impl();
  Impl(const std::string& init_foo);
  ~Impl() = default;
  Impl(const Impl&) = delete;
  Impl& operator=(const Impl&) = delete;

  // Use a template because we don't care which Lockable concept or LockType
  // is being used, just so long as a lock is held.
  template <typename LockType>
  bool bar(LockType& lk, std::size_t len, char* dst) const;

  template <typename LockType>
  std::size_t bar_capacity(LockType& lk) const;

  // bar_set() requires a unique lock
  bool bar_set(unique_lock_t& lk, const std::size_t len, const char* src);

  template <typename LockType>
  std::string foo(LockType& lk) const;

  template <typename T>
  bool foo_set(unique_lock_t& lk, T&& src);

private:
  // Example datatype that supports rvalue references
  std::string foo_;

  // Example POD datatype that doesn't support rvalue
  static const std::size_t bar_capacity_ = 16;
  char bar_[bar_capacity_ + 1];
};

// Example delegating ctor
Example::Impl::Impl() : Impl("default foo value") {}

Example::Impl::Impl(const std::string& init_foo) : foo_{init_foo} {
  std::memset(bar_, 99 /* ASCII 'c' */, bar_capacity_);
  bar_[bar_capacity_] = '\0'; // null padding
}


template <typename LockType>
bool
Example::Impl::bar(LockType& lk, const std::size_t len, char* dst) const {
  BOOST_ASSERT(lk.owns_lock());
  if (len != bar_capacity(lk))
    return false;
  std::memcpy(dst, bar_, len);

  return true;
}


template <typename LockType>
std::size_t
Example::Impl::bar_capacity(LockType& lk) const {
  BOOST_ASSERT(lk.owns_lock());
  return Impl::bar_capacity_;
}


bool
Example::Impl::bar_set(unique_lock_t &lk, const std::size_t len, const char* src) {
  BOOST_ASSERT(lk.owns_lock());

  // Return false if len is bigger than bar_capacity or the values are
  // identical
  if (len > bar_capacity(lk) || foo(lk) == src)
    return false;

  // Copy src to bar_, a side effect of updating foo_ if they're different
  std::memcpy(bar_, src, std::min(len, bar_capacity(lk)));
  foo_set(lk, std::string(src, len));
  return true;
}


template <typename LockType>
std::string
Example::Impl::foo(LockType& lk) const {
  BOOST_ASSERT(lk.owns_lock());
  return foo_;
}


template <typename T>
bool
Example::Impl::foo_set(unique_lock_t &lk, T&& src) {
  BOOST_ASSERT(lk.owns_lock());
  if (foo_ == src) return false;
  foo_ = std::move(src);
  return true;
}


// Example Public Interface

Example::Example() : impl_(new Impl{}) {}
Example::Example(const std::string& init_foo) : impl_(new Impl{init_foo}) {}
Example::~Example() = default;

bool
Example::bar(const std::size_t len, char* dst) const {
  shared_lock_t lk(rw_mtx_);
  return impl_->bar(lk, len , dst);
}

std::size_t
Example::bar_capacity() const {
  shared_lock_t lk(rw_mtx_);
  return impl_->bar_capacity(lk);
}

bool
Example::bar_set(const std::size_t len, const char* src) {
  unique_lock_t lk(rw_mtx_);
  return impl_->bar_set(lk, len, src);
}

std::string
Example::foo() const {
  shared_lock_t lk(rw_mtx_);
  return impl_->foo(lk);
}

template<typename T>
bool
Example::foo_set(T&& src) {
  unique_lock_t lk(rw_mtx_);
  return impl_->foo_set(lk, std::forward<T>(src));
}

} // namespace stackoverflow

And my questions are:

1. Is there a better way to handle locking inside of the private implementation?
2. Is there any actual harm in making Impl public given the definition is opaque?
3. When using clang's -O4 to enable Link-Time Optimization, it should be possible for the linker to bypass the dereference overhead of std::unique_ptr. Has anyone verified that?
4. Is there a way to call foo_set("asdf") and have the example link correctly? We're having problems figuring out what the correct explicit template instantiation is for const char[6]. For now I've setup a template specialization that creates a std::string object and proxies a call to foo_set(). All things considered, this seems like the best way forward, but I would like to know how to pass "asdf" and std::decay the result.

Regarding the locking strategy, I've developed an obvious bias towards this for several reasons:

• I can change out the mutex to be an exclusive mutex where appropriate
• By designing the Impl API to include the required lock, there is a very strong compile-time guarantee of the locking semantics
• It is difficult to forget to lock something (and a "simple API" bug when this does happen, again the compiler will catch this once the API has been fixed)
• It is difficult to leave something locked or create a dead lock due to RAII and having the Impl not have a reference to the mutex
• Use of templates removes the need to downgrade from a unique lock to a shared lock
• Because this locking strategy covers more code than is actually required, it requires explicit effort to downgrade a lock from unique to shared, which handles the all-too-common scenarior where assumptions made with a shared lock need to be retested upon entering a unique locked area
• Bug fixes or Impl API changes don't require recompiling the entire application since example.hpp's API is externally fixed.

I've read that ACE uses this type of locking strategy as well, but I'm welcome some realworld criticism from ACE users or others re: passing the lock around as a required part of the interface.

For the sake of completeness, here's an example_main.cpp for folks to chew on.

#include <sysexits.h>

#include <cassert>
#include <iostream>
#include <memory>
#include <stdexcept>

#include "example.hpp"

int
main(const int /*argc*/, const char** /*argv*/) {
  using std::cout;
  using std::endl;
  using stackoverflow::Example;

  {
    Example e;
    cout << "Example's foo w/ empty ctor arg: " << e.foo() << endl;
  }

  {
    Example e("foo");
    cout << "Example's foo w/ ctor arg: " << e.foo() << endl;
  }

  try {
    Example e;
    { // Test assignment from std::string
      std::string str("cccccccc");
      e.foo_set(str);
      assert(e.foo() == "cccccccc");  // Value is the same
      assert(str.empty());            // Stole the contents of a_str
    }
    { // Test assignment from a const std::string
      const std::string const_str("bbbbbbb");
      e.foo_set(const_str);
      assert(const_str == "bbbbbbb");               // Value is the same
      assert(const_str.c_str() != e.foo().c_str()); // Made a copy
    }
    {
      // Test a const char[7] and a temporary std::string
      e.foo_set("foobar");
      e.foo_set(std::string("ddddd"));
    }
    { // Test char[7]
      char buf[7] = {"foobar"};
      e.foo_set(buf);
      assert(e.foo() == "foobar");
    }
    { //// And a *char[] & const *char[]
      // Use unique_ptr to automatically free buf
      std::unique_ptr<char[]> buf(new char[7]);
      std::memcpy(buf.get(), "foobar", 6);
      buf[6] = '\0';
      e.foo_set(buf.get());
      const char* const_ptr = buf.get();
      e.foo_set(const_ptr);
      assert(e.foo() == "foobar");
    }

    cout << "Example's bar capacity: " << e.bar_capacity() << endl;
    const std::size_t len = e.bar_capacity();

    std::unique_ptr<char[]> buf(new char[len +1]);

    // Copy bar in to buf
    if (!e.bar(len, buf.get()))
      throw std::runtime_error("Unable to get bar");
    buf[len] = '\0'; // Null terminate the C string
    cout << endl << "foo and bar (a.k.a.) have different values:" << endl;
    cout << "Example's foo value: " << e.foo() << endl;
    cout << "Example's bar value: " << buf.get() << endl;

    // Set bar, which has a side effect of calling foo_set()
    buf[0] = 'c'; buf[1] = buf[2] = '+'; buf[3] = '\0';
    if (!e.bar_set(sizeof("c++") - 1, buf.get()))
      throw std::runtime_error("Unable to set bar");

    cout << endl << "foo and bar now have identical values but only one lock was acquired when setting:" << endl;
    cout << "Example's foo value: " << e.foo() << endl;
    cout << "Example's bar value: " << buf.get() << endl;
  } catch (...) {
    return EX_SOFTWARE;
  }

  return EX_OK;
}

And build instructions to use C++11 and libc++:

clang++ -O4 -std=c++11 -stdlib=libc++ -I/path/to/boost/include -o example.cpp.o -c example.cpp
clang++ -O4 -std=c++11 -stdlib=libc++ -I/path/to/boost/include -o example_main.cpp.o -c example_main.cpp
clang++ -O4 -stdlib=libc++ -o example example.cpp.o example_main.cpp.o /path/to/boost/lib/libboost_exception-mt.dylib /path/to/boost/lib/libboost_system-mt.dylib /path/to/boost/lib/libboost_thread-mt.dylib

---

As a small bonus, I updated this example to include perfect forwarding using rvalue references in the foo_set() method. While not perfect, it took longer than I anticipated to get the template instantiation correct which is an issue when linking. This also includes the appropriate overloads for C basic types, including: char*, const char*, char[N], and const char[N].

Answer 1

For question 1, one thing I'd be tempted to do is use SFINAE to restrict the lock types passed in as LockType allowed to shared_lock_t or unique_lock_t.

Ie:

template <typename LockType>
typename std::enable_if<
  std::is_same< LockType, shared_lock_t > || std::is_same< LockType, unique_lock_t >,
  size_t
>::type 
bar_capacity(LockType& lk) const;

... but that does get a bit verbose.

which means that passing in the wrong type of Lock gives you a "nothing matches" error. Another approach would be to have two different bar_capacity that take shared_lock_t and unique_lock_t exposed, and a private bar_capacity that they that takes a template LockType.

As written, any type with a .owns_lock() method that returns a type convertible to bool is a valid argument there...

Answer 2

Using a Pimpl idiom, the mutex should be part of the implementation. This will let you to master when the lock is started.

BTW, why using a unique_lock when a lock_guard will be enough?

I don't see any advantage to make impl public.

std::unique_ptr should be as efficient as a pointer for most of the moderns compilers. Not verified however.

I would forward the const char[N] foo_set not as

  template<std::size_t N>
  bool foo_set(const char (&new_val)[N]) { return foo_set(std::string(new_val, N)); }

but like

  template<std::size_t N>
  bool foo_set(const char (&new_val)[N]) { return foo_set(N, new_val); }

This avoids the string creation on the header file and let the implementation do whatever is needed.