Copy constructor with non-const argument suggested by thread-safety rules?

Question

I have a wrapper to some piece of legacy code.

class A{
   L* impl_; // the legacy object has to be in the heap, could be also unique_ptr
   A(A const&) = delete;
   L* duplicate(){L* ret; legacy_duplicate(impl_, &L); return ret;}
   ... // proper resource management here
};

In this legacy code, the function that “duplicates” an object is not thread safe (when calling on the same first argument), therefore it is not marked const in the wrapper. I guess following modern rules: https://herbsutter.com/2013/01/01/video-you-dont-know-const-and-mutable/

This duplicate looks like a good way to implement a copy constructor, except for the detail that is it not const. Therefore I cannot do this directly:

class A{
   L* impl_; // the legacy object has to be in the heap
   A(A const& other) : L{other.duplicate()}{} // error calling a non-const function
   L* duplicate(){L* ret; legacy_duplicate(impl_, &ret); return ret;}
};

So what is the way out this paradoxical situation?

(Let's say also that legacy_duplicate is not thread-safe but I know leaves the object in the original state when it exits. Being a C-function the behavior is only documented but has no concept of constness.)

I can think of many possible scenarios:

(1) One possibility is that there is no way to implement a copy constructor with the usual semantics at all. (Yes, I can move the object and that is not what I need.)

(2) On the other hand, copying an object is inherently non-thread-safe in the sense that copying a simple type can find the source in an half-modified state, so I can just go forward and do this perhaps,

class A{
   L* impl_;
   A(A const& other) : L{const_cast<A&>(other).duplicate()}{} // error calling a non-const function
   L* duplicate(){L* ret; legacy_duplicate(impl_, &ret); return ret;}
};

(3) or even just declare duplicate const and lie about thread safety in all contexts. (After all the legacy function doesn't care about const so the compiler will not even complain.)

class A{
   L* impl_;
   A(A const& other) : L{other.duplicate()}{}
   L* duplicate() const{L* ret; legacy_duplicate(impl_, &ret); return ret;}
};

(4) Finally, I can follow the logic and make a copy-constructor that takes a non-const argument.

class A{
   L* impl_;
   A(A const&) = delete;
   A(A& other) : L{other.duplicate()}{}
   L* duplicate(){L* ret; legacy_duplicate(impl_, &ret); return ret;}
};

It turns out that this works in many contexts, because these objects are not usually const.

The question is, it this a valid or common route?

I cannot name them, but I intuitively expect lots of problems down the road of having a non-const copy constructor. Probably it will not qualify as a value-type because of this subtlety.

(5) Finally, although this seems to be an overkill and could have a steep runtime cost, I could add a mutex:

class A{
   L* impl_;
   A(A const& other) : L{other.duplicate_locked()}{}
   L* duplicate(){
      L* ret; legacy_duplicate(impl_, &ret); return ret;
   }
   L* duplicate_locked() const{
      std::lock_guard<std::mutex> lk(mut);
      L* ret; legacy_duplicate(impl_, &ret); return ret;
   }
   mutable std::mutex mut;
};

But being forced to do this looks like pessimization and makes the class bigger. I am not sure. I am currently leaning towards (4), or (5) or a combination of both.

---

EDIT 1:

Another option:

(6) Forget about all the non-sense of the duplicate member function and simply call legacy_duplicate from the constructor and declare that the copy constructor is not thread safe. (And if necessary make another thread-safe versión of the type, A_mt)

class A{
   L* impl_;
   A(A const& other){legacy_duplicate(other.impl_, &impl_);}
};

---

EDIT 2:

This could be a good model for what the legacy function does. Note that by touching the input the call is not thread safe with respect to the value represented by the first argument.

void legacy_duplicate(L* in, L** out){
   *out = new L{};
   char tmp = in[0];
   in[0] = tmp; 
   std::memcpy(*out, in, sizeof *in); return; 
}

---

EDIT 3: I lately learned that std::auto_ptr had a similar problem of having a non-const "copy" constructor. The effect was that auto_ptr couldn't be used inside a container. https://www.quantstart.com/articles/STL-Containers-and-Auto_ptrs-Why-They-Dont-Mix/

Fridge

"In this legacy code the function that duplicates an object is not thread safe (when calling on the same first argument)" Are you sure about that? Is there some state not contained within L which is modified by creating a new L instance? If not, why do you believe that this operation is not thread-safe? – Eiten

Yes, that is the situation. It looks like the internal state of the first argument is modified during the exection. For some reason (some "optimization" or bad design or simply by specification) the function legacy_duplicate cannot be called with the same first argument from two different threads. – Fridge

@TedLyngmo ok I did. Although technically in c++ pre 11 const has a more fuzzy meaning in the presence of threads. – Fridge

@TedLyngmo yes, it is a pretty good video. it is a pity that the video just deals with proper members and doesn’t touch on the problem of construction (were also the constness is on the “other” object). In perspective there might be no intrinsic way of making this wrapper thread safe upon copying without adding another layer of abstraction (and a concrete mutex). – Fridge

Yes, well, that got me confused and I'm probably one of those people who doesn't know what const really means. :-) I wouldn't think twice about taking a const& in my copy ctor as long as I don't modify other. I always think of thread safety as something one adds on top of whatever needs to be accessed from multiple threads, via encapsulation, and I'm really looking forward to the answers. – Ruyle

This isn't an answer but may help some. Is the usage of impl_ entirely encapsulated in this class? If yes then won't you need the mutex anyway to control other access to impl_ in which case option 5 might be best? if no, then you've lost control of threading issues anyway. Do you really need a copy constructor or would a move constructor (en.cppreference.com/w/cpp/language/move_constructor) be enough and avoid all of the copying? – Lenee

@Lenee very good point. i put a raw pointer in the example to simplify. In reality I have a unique_ptr of “unique” management of the raw pointer. As far as I understand unique management doesn’t need per se synchronization/mutex. If It was a shared pointer which is another possible design then yes your point would be obviously valid. – Fridge

@alfC: I'm not sure what "authoritative references" can exist for something that is at best a convention, not a rule of the language. – Eiten

My first point was that if legacy_duplicate is not thread safe that there are likely other method calls that you are trying to encapsulate which are also not thread safe. If so, then you'll probably want to use a mutex to ensure thread safety for these other methods as well. If you're going that far, then using the mutex for legacy_duplicate would just be another case where you need to ensure thread safety. – Lenee

@NicolBolas, arguably, I think Google style guide (as in Michael's answer) or similar can be considered an authoritative reference (of common or accepted use at least). – Fridge

Answer 1

I would just include both your options (4) and (5), but explicitly opt-in to thread-unsafe behavior when you think it is necessary for performance.

Here is a complete example.

#include <cstdlib>
#include <thread>

struct L {
  int val;
};

void legacy_duplicate(const L* in, L** out) {
  *out = new L{};
  std::memcpy(*out, in, sizeof *in);
  return;
}

class A {
 public:
  A(L* l) : impl_{l} {}
  A(A const& other) : impl_{other.duplicate_locked()} {}

  A copy_unsafe_for_multithreading() { return {duplicate()}; }

  L* impl_;

  L* duplicate() {
    printf("in duplicate\n");
    L* ret;
    legacy_duplicate(impl_, &ret);
    return ret;
  }
  L* duplicate_locked() const {
    std::lock_guard<std::mutex> lk(mut);
    printf("in duplicate_locked\n");
    L* ret;
    legacy_duplicate(impl_, &ret);
    return ret;
  }
  mutable std::mutex mut;
};

int main() {
  A a(new L{1});
  const A b(new L{2});

  A c = a;
  A d = b;

  A e = a.copy_unsafe_for_multithreading();
  A f = const_cast<A&>(b).copy_unsafe_for_multithreading();

  printf("\npointers:\na=%p\nb=%p\nc=%p\nc=%p\nd=%p\nf=%p\n\n", a.impl_,
     b.impl_, c.impl_, d.impl_, e.impl_, f.impl_);

  printf("vals:\na=%d\nb=%d\nc=%d\nc=%d\nd=%d\nf=%d\n", a.impl_->val,
     b.impl_->val, c.impl_->val, d.impl_->val, e.impl_->val, f.impl_->val);
}

Output:

in duplicate_locked
in duplicate_locked
in duplicate
in duplicate

pointers:
a=0x7f85e8c01840
b=0x7f85e8c01850
c=0x7f85e8c01860
c=0x7f85e8c01870
d=0x7f85e8c01880
f=0x7f85e8c01890

vals:
a=1
b=2
c=1
c=2
d=1
f=2

This follows the Google style guide in which const communicates thread safety, but code calling your API can opt-out using const_cast

Boisterous

Thank you for the answer, I think it doesn't change your asnwer and I am not sure but a better model for legacy_duplicate could be void legacy_duplicate(L* in, L** out) { *out = new L{}; char tmp = in[0]; /*some weird call here*/; in[0] = tmp; std::memcpy(*out, in, sizeof *in); return; } (i.e. non-const in) – Fridge

Your answer is very interesting because it can be combined with option (4) and an explicit version of option (2). That is, A a2(a1) could try to be thread safe (or be deleted) and A a2(const_cast<A&>(a1)) wouldn't try to be thread safe at all. – Fridge

Yes, if you plan to use A in both thread-safe and thread-unsafe contexts, you should pull the const_cast to the calling code so that it is clear where thread-safety is known to be violated. It is okay to push extra safety behind the API (mutex) but not okay to hide unsafety (const_cast). – Boisterous

Answer 2

TLDR: Either fix the implementation of your duplication function, or introduce a mutex (or some more appropriate locking device, perhaps a spinlock, or make sure your mutex is configured to spin before doing anything heavier) for now, then fix the implementation of duplication and remove the locking when the locking actually becomes a problem.

I think a key point to notice is that you are adding a feature that did not exist before: the ability to duplicate an object from multiple threads at the same time.

Obviously, under the conditions you have decribed, that would have been a bug - a race condition, if you had been doing that before, without using some kind of external synchronization.

Therefore, any use of the this new feature will be something you add to your code, not inherit as existing functionality. You should be the one who knows whether adding the extra locking will actually be costly - depending on how often you are going to be using this new feature.

Also, based on the perceived complexity of the object - by the special treatment you are giving it, I'm going to assume that the duplication procedure is not a trivial one, therefore, already quite expensive in terms of performance.

Based on the above, you have two paths you can follow:

A) You know that copying this object from multiple threads will not happen often enough for the overhead of the additional locking to be costly - perfhaps trivially cheap, at least given that the existing duplication procedure is costly enough on its own, if you use a spinlock/pre-spinning mutex, and there is no contention on it.

B) You suspect that copying from multiple threads will happen often enough for the extra locking to be a problem. Then you really have only one option - fix your duplication code. If you don't fix it, you will need locking anyway, whether at this layer of abstraction or somewhere else, but you will need it if you don't want bugs - and as we've established, in this path, you assume that locking will be too costly, therefore, the only option is fixing the duplication code.

I suspect that you are really in situation A, and just adding a spinlock/spinning mutex that has close to no performance penalty when uncontested, will work just fine (remember to benchmark it, though).

There is, in theory, another situation:

C) In contrast to the seeming complexity of the duplication function, it is actually trivial, but can't be fixed for some reason; it is so trivial that even an uncontested spinlock introduces an unacceptable performance degradation to duplication; duplication on parallell threads is used rarely; duplication on a single thread is used all the time, making the performance degradation absolutely unacceptable.

In this case, I suggest the following: declare the default copy constructors / operators deleted, to prevent anyone from using them accidentally. Create two explicitly callable duplication methods, a thread safe one, and a thread unsafe one; make your users call them explicitly, depending on context. Again, there is no other way to achieve acceptable single thread performance and safe multi threading, if you really are in this situation and you just can't fix the existing duplication implementation. But I feel it is highly unlikely that you really are.

Just add that mutex/spinlock and benchmark.

Freetown

Can you point me to material on spinlock/pre-spinning mutex in C++? Is it something more complicated that what is provided by std::mutex? The duplicate function is no secret, I didn't mention it to keep the problem at a high level and not receive answers about MPI. But since you went that deep I can give you more details. The legacy function is MPI_Comm_dup and the effective non-thread safeness is described here (I confirmed it) github.com/pmodels/mpich/issues/3234. This is why I cannot fix duplicate. (Also, if I add a mutex I will be tempted to make all the MPI calls thread-safe.) – Fridge

Sadly I don't know much std::mutex, but I guess it does some spinning before letting the process sleep. A well known synchronization device where you get to control this manually, is: docs.microsoft.com/en-us/windows/win32/api/synchapi/… I haven't compared performance, but it seems that std::mutex is now superior: #9997973 and implemented using: docs.microsoft.com/en-us/windows/win32/sync/… – Freetown

It seems this is good description of the general considerations to take into account: #5870325 – Freetown

Thanks again, I am in Linux if that matters. – Fridge

Here is a somewhat detailed performance comparison (for a different language, but I guess this is informative and indicative of what to expect): matklad.github.io/2020/01/04/… The TLDR is - spinlocks win by an extremely small margin when there is no contention, can lose badly when there is contention. – Freetown