Helllo, I am multithreading large piece of code (gcc 7.4, c++17) and I stumbled upon a lot of race conditions when accessing std::shared_ptr. 
I tried using boost::atomic_shared_ptr, but there are some critical parts of the code where shared pointers are returned from functions, or shared pointers are used in constructors of other classes, which is not supported from boost::atomic_shared_ptr (copy constructor and assignment operator are private). 
I had 3 other suggestions: 
-to wrap every place where shared pointer is accessed with mutex, but this looks like overkill to me.
-to create my implementation of atomic_shared_pointer. 
-to use std::atomic_store/std::atomic_load. 

What is the best way to make the access to the shared pointers thread-safe? Is there something special to consider when returning shared pointer from function, related to atomicity?

Try to read this. That might help.

IMHO std::shared_ptr sucks anyway since it's typically twice the size of a regular pointer and has a large control block to boot.  Even if you find a way to make std::shared_ptr thread safe, I would roll my own anyway because it's easy to do so.  To be fair however I use a lot of smart pointers so it's a big deal for me. Depending on your exact application you may not care that much.

LOL! Got down voted for not liking std::shared_ptr. I remember it the days of STL some people would go into a rage if you said anything bad about it.

I have little experience with std::shared_ptr, only use it in very view places, but any info interests me.

First, my question would be what is the exact problem with Gnollrunner's opinion on shared_ptr ? I have seen quite a few "home brews" of shared or reference counted pointers and that not only in ancient code. It would be nice, for the less experienced among us ? to know the reason, if it is technical.

For my naive understanding the so called smart pointers are all about ownership, not about regulating access to their objects. It is application responsibility to ensure that no undefined behaviour happens, in this case to take care of possible race conditions, like one would do with any other object, except for the reference counter and object destruction, that's responsibility of the shared_ptr. Am i wrong ?

Edit: but https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic2

6 hours ago, svetpet said:

What is the best way to make the access to the shared pointers thread-safe?

1

I guess writing your own implementation shouldn't be too hard and its the best way to get exactly what you need. You can also just copy the code of the boost or STL shared pointer and adjust it to your needs.

6 hours ago, svetpet said:

wrap every place where shared pointer is accessed with mutex, but this looks like overkill to me

You could also use a wrapper class for that. In case you are going this route, you should probably use a spinlock instead of a mutex. A mutex might send your thread to sleep which results in a huge performance hit. A spinlock keeps trying to access the data until it is available. This is assuming that you want to use only as many threads as processors are available. If you are using more threads (wonder why I should do that in a game engine), a mutex is fine.

27 minutes ago, Green_Baron said:

First, my question would be what is the exact problem with Gnollrunner's opinion on shared_ptr ? I have seen quite a few "home brews" of shared or reference counted pointers and that not only in ancient code. It would be nice, for the less experienced among us ? to know the reason, if it is technical. 

Well, it's just an opinion, which somebody disliked for some reason. Personally, I also don't like shared pointers. They seem always like an easy solution to many problems, but most of the times they just make your code way more complicated since object lifetime gets much harder to predict. In the worst case, you create a circular dependency which results in a really mean kind of memory leak. However, there are situations where they are really a good choice. It's like always: Select the correct tool for your problem.

1 hour ago, Gnollrunner said:

LOL! Got down voted for not liking std::shared_ptr. I remember it the days of STL some people would go into a rage if you said anything bad about it.

You even stated in the beginning that it's your opinion... well, just neutralized it

Greetings

2 hours ago, DerTroll said:

I guess writing your own implementation shouldn't be too hard and its the best way to get exactly what you need.

That's effectively not hard once you know what reference-counting smart pointers are. But why ? This will definitely be incompatible with standard smart pointers. Also, the OP has issues only with multithreading. And there exists ways to use std smart pointers well in multithreaded environments (fortunately). Also, making everything thread-safe is certainly not the best choice. And smart pointers are already slower than raw pointers. So why making them even more slow ?

2 hours ago, DerTroll said:

You can also just copy the code of the boost or STL shared pointer and adjust it to your needs.

Depending on what the clauses of the licenses would allow.

2 hours ago, DerTroll said:

Personally, I also don't like shared pointers. They seem always like an easy solution to many problems, but most of the times they just make your code way more complicated since object lifetime gets much harder to predict. In the worst case, you create a circular dependency which results in a really mean kind of memory leak. However, there are situations where they are really a good choice. It's like always: Select the correct tool for your problem.

Unfortunately SP are well sit on the road nowadays. The fact that we like them or not might not suffice depending on which projects we are working with. I personally try to avoid them too since they have a performance consideration, did not prove to get rid of memory leaks, neither to ease the memory use, and they can easily get very hard to use once you want to use them well (ie in multithreading, or with using other smart pointers as unique_ptr or so, or to cast them, or in hybrid environments...). At the end, you get them everywhere, in different forms (ie having both typenames for unique an shared ptr in any classes, casts everywhere, copies everywhere, nullptr tests everywhere...).

I am not the one who down-voted (I dislike this since it is mainly too easily used and mostly cowardly used), but the answer of Gnollrunner was out-of-topic since OP clearly stated that:

9 hours ago, svetpet said:

I am multithreading large piece of code

So this is not suitable to ask the OP to simply remove his smart pointer uses neither to tell we dislike them.

Finally using well smart pointers is not an easy piece. There are good books treating about this. And this involves to use more broadly the standard library.

4 hours ago, _Silence_ said:

 

I am not the one who down-voted (I dislike this since it is mainly too easily used and mostly cowardly used), but the answer of Gnollrunner was out-of-topic since OP clearly stated that:

So this is not suitable to ask the OP to simply remove his smart pointer uses neither to tell we dislike them.

First off the OP even stated as one of his options:
 

Quote

-to create my implementation of atomic_shared_pointer. 

 So I'm not sure why you think I was "out-of-topic". I was pointing out that std:shared_ptr has other downsides in addition to the threading issues, that can be done away with if he wrote his own version.  Also unlike some respondents here, I in fact do like and use smart pointers and never suggested that he remove them. I have used them since the early 90s.  Again, I was recommending one of the options that the OP himself listed. I'll quote myself:

Quote

I would roll my own anyway because it's easy to do so.

Also in regards to this:

Quote

Finally using well smart pointers is not an easy piece. There are good books treating about this. And this involves to use more broadly the standard library.

I  find smart pointers (note: I didn't say std::shared_ptr) quite easy to use.  IMO they are a good solution where you have multiple ownership of objects.  However there is nothing that says you must use the standard library version, which again has a pointer that is typically 2X the size of a raw pointer and in addition has a large control block associated with each object.

13 hours ago, svetpet said:

I had 3 other suggestions: 
-to wrap every place where shared pointer is accessed with mutex, but this looks like overkill to me.
-to create my implementation of atomic_shared_pointer. 
-to use std::atomic_store/std::atomic_load.

I don't believe that the exact nature of your race condition(s) was ever described?  Is the problem actually in the pointers themselves, or is it in the access to the target of the pointer?  This is a fairly critical point to understand before doing much more than generalization.  If simply moving the pointers themselves around is the problem then changing the pointer to be custom is unlikely to be a valid fix as it is most likely a usage pattern issue rather than something a change would solve.  On the other hand, if it is a problem with accessing what is being pointed 'to' (the target), that's a very different story.

I'm going to assume it is access to the target that is the problem so as a generalization, there is a simple rule which is being broken when you talk about shared_ptr in multi-threaded code: there can only be one thread that owns data at a time, ever.  Breaking this rule is the primary cause of race conditions, dead locks and data corruptions in pretty much every piece of software that attempts to be threaded.  In fact, this is so common there is an entire language which enforces the rule at it's base most level.  As someone pointed out, there are ways around this which folks have found but generally speaking they tend to be either slow to the point that you should have stayed single threaded or complicated and/or buggy.

The first solution you mention may seem overkill but it is actually the only way you will achieve any generic level of safety.  Basically I've seen this done via wrapping shared_ptr in a class which also references out to a shared mutex.  Any copy of the pointer made gets a reference to that shared mutex.  Dereference operations are removed and now you must call lock to get the pointer, which locks the mutex, and unlock to free the mutex and preferably invalidate the pointer kinda like a weak_ptr->shared_ptr->drop.  This will guarantee that only one thread can access the memory at any given time and likely solve a great number of problems.  Unfortunately though, you have an extremely high likelihood of running into dead locks and race conditions.

Large or small codebase doesn't really change anything unfortunately.  If you don't correct the data ownership up front you are going to likely have so many problems later that it takes longer to finish the work than investing the time up front. Additionally, the result could very possibly end up slower rather than the single threaded code because you'll have to patch so many holes in the code with checks and additional lock layers etc.  Shared, mutable, data is the absolute bane of multithreaded code.