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

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.