Unsafe uses of callAsync


#1
void SomeComponent::notifyStatusChanged() // called from any thread
{
	MessageManager::getInstance()->callAsync([]()
	{
		listeners.call(&Listener::statusChanged);
	});
}

I’m thinking this is an unsafe race condition?

There might be something in the message manager queue which deletes the SomeComponent before the listeners.call is executed?

A problem which is avoided using AsyncUpdater etc.


#2
void SomeComponent::notifyStatusChanged() // called from any thread
{
    Component::SafePointer<SomeComponent> sp (this);
    MessageManager::getInstance()->callAsync ([sp]()
    {
        if (sp != nullptr)
	    listeners.call(&Listener::statusChanged);
    });
}

#3

But generally you have more of a race condition if notifyStatusChanged is called from any thread. You’ll need to guard access to the creation of the SafePointer as they are not thread safe.

In general, yes use an async updater to be on the safe side.


#4

Cheers Dave, though it looked iffy, but didn’t get enough sleep last night.

class MultiAsyncUpdater
	:
AsyncUpdater
{
public:
	MultiAsyncUpdater()
	{}

	void callOnMessageThread(std::function<void()> callback)
	{
		ScopedLock l(lock);
		queue.push_back(callback);
		triggerAsyncUpdate();
	}

private:
	void handleAsyncUpdate() override
	{
		ScopedLock l(lock);

		for (auto & func : queue)
			func();

		queue.clear();
	}

	CriticalSection lock;
	std::vector<std::function<void()>> queue;
};

I’m thinking this is the general purpose solution… obviously lots of other considerations if performance is critical and it’s gong to be called really frequently…but i think it’ll be good.


#5

Yes, that should work as long as you don’t delete the MultiAsyncUpdater concurrently with calling callOnMessageThread.
Also, for a small speed increase you probably want to move callback into the vector:
queue.push_back (std::move (callback));


#6

Good point. Though you’ve forced me to open up a whole can of worms here now instead of getting the job finished…


#7

I’m going to talk about this next week…
In general though I’ve seen a lot of bugs arise from passing std::function by reference. If you’re going to make a copy anyway you might as well make the copy when passing the argument then move the parameter. Then, it’s up to the caller to move the argument if they don’t need it anymore. I’ve not looked in to it fully but I’m also hoping that one day compilers will be able to spot xvalues and automatically move them into arguments if they are values not references.


#8

Bloody hell … that MultiAsyncUpdater class just crashed! :slight_smile: Thought that was looking safe… maybe a reentrancy problem where a callback then called the callOnMessageThread function…