Determine concurrent access to a function

Question

Someone asked here how to determine a function is being called from multiple threads. My take on this is that they are asking about concurrent access not sequential access. The accepted answer therefore seems inadequate. Detecting concurrent access is also a problem I need to solve for debugging purposes so I thought I'd take a stab at a solution.

This takes an RAII-style approach to the solution, requiring only an instantiation of the class at the top of the function in question.

template <std::ostream& stream> class ConcurrentPrint: public std::ostringstream
{
public:
 ~ConcurrentPrint()
 {
 std::scoped_lock lock(printMutex);
 stream << this->str();
 }
private:
 static std::mutex printMutex;
};
template <std::ostream& stream> std::mutex ConcurrentPrint<stream>::printMutex;
class ConcurrencyChecker
{
public:
 ConcurrencyChecker(const std::string& id_) :
 id(id_)
 {
 std::scoped_lock lock(mapMutex);
 if (++numInstances[id] > 1)
 {
 ConcurrentPrint<std::cerr>() << "Warning: concurrent access to " << id << " from " << std::this_thread::get_id() << std::endl;
 abort();
 }
 }
 ~ConcurrencyChecker()
 {
 std::scoped_lock lock(mapMutex);
 --numInstances[id];
 }
private:
 static std::map<std::string, int> numInstances;
 static std::mutex cerrMutex;
 const std::string id;
 std::mutex mapMutex;
};
std::map<std::string, int> ConcurrencyChecker::numInstances = {};

Intended usage:

void functionToCheck()
{
 ConcurrencyChecker check(__func__);
 /* Function body */
}

Working example here

Things I'm interested in:

• Are there any flaws in this general approach and is there a better one?
• Any general improvements to the code

• Toby Speight: Are you missing a definition of ConcurrencyChecker::cerrMutex?
• j b: @TobySpeight good catch. cerrMutex is actually unneeded and just residual from before I broke ConcurrentPrint into its own class.
• sudo rm -rf slash: This is a non deterministic detector. Even if the caller does everything she can to cause concurrent access, it might not get caught by your detector (even your test is not deterministic -- suppose the ith thread yields for 10*i seconds immediately after being called and there are no other breaks). In my opinion, you're better off proving the fn can/cannot be called concurrently at design time rather than trying to debug it at runtime. There are tools that can automate this.
• Josiah: The core of the claim here is that while your construction can tell you that you do have a problem, it cannot tell you that you do not. That is to say, it is possible to run your thing in testing a dozen times and it be fine, but the code could still exhibit race conditions that happen on run number twenty-four. Therefore although it can be helpful, especially for detection of frequent collisions, it should not be relied on alone.
• Josiah: A macro actually wouldn't materially change the point made. You don't care about whether two threads are technically running the same function. You do care whether they are might be doing mutually dangerous operations, and being in the same function is shorthand for that. Therefore, whether the check is technically before or after the function call is less important than that it is before any dangerous stuff.

Answer 1

A few observations off the top of my head

cerrMutex is unused, and should be removed if not needed.

mapMutex is used to guard a static std::map but is itself a regular class member variable. That suggests it probably isn't guarding the map against concurrent access. This is probably a bug.

This seems like a fairly heavyweight tool. The uses of maps and strings all smell bloaty. That's not of itself a killer, but it's worth being at least aware of. If there were the possibility of just using, say, object-local integers for counting (and perhaps a function scope static declaration to use the same counting variables), that would probably be lighter.

On the other hand, in a general use case, concurrency causes problems whenever two threads are interacting with the same resource, not only when executing the same function. For example this tool would be significantly more useful if it could detect different threads concurrently running two methods of a single object.
At the same time, it would be more helpful if it actually did not interfere with two threads each holding their own object of a given class and each running methods on their own object. This is perhaps more of a documentation enhancement suggestion: your rough approach could work by changing what ID you pass, but it would benefit from examples. (And don't go changing it to make it lightweight if it means losing the ability to do this!)

---

A final thing that is worth thinking about is whether this causes any inadvertant synchronisation that actually masks the very problems you're trying to detect. This is hard to predict, because schedulers can be such a dark art in general, but you could run into a problem like this.

Suppose that you have two threads running a function that does something lightweight, perhaps just increment a shared integer. Without this tool in place, you'd expect them to keep stomping on each other.

Because you're syncing up on mapMutex (which I'm going to assume as above is meant to be static) then each call to the function has to do a lot of extra work in the constructor and destructor of your tool, and that is all under exclusion. If thread 2 starts while thread 1 is still in the constructor, it will fail to take mapMutex and the scheduler will probably send it to sleep for a bit. The window for it to wake up in which thread 1 is in the function and does not have the mutex is actually quite small. What you could get, then, is the function seems to behave fine (albeit slowly) while this object is trying to help with debugging and then goes back into data races for release code when the debug object is removed. Suffice to say, that's unhelpful!

I don't have any suggestions about how to fix this problem, just that it's the sort of thing that is worth being aware of.

• j b: thanks for the helpful feedback. I used the map to facilitate differentiating usage from different functions. An unordered_map<int, int> using integer ids would be lighter but then you lose the option to identify the caller by name. OTOH, taking on board your overall points, I wonder if the problem might be better solved with a macro...