[core, ios, qt, android] Close race condition in RunLoop (issue #9620)

[ChrisLoer]

See #9620 (comment)

Because a message we queue from the foreground may cause the background to complete, exit, and tear down the AsyncTask, we have to block queue processing until we've finished our call to AsyncTask::send().

Broadening the scope of a mutex is scary, but I audited the code of our four implementations of AsyncTask and I don't see any way this could cause a deadlock.

If we wanted to keep the scope of the mutex as limited as possible, we could move the mutex scope-broadening to RunLoop::stop(), with a custom implementation that bypasses invoke/push, but I'm not sure it would be worth the extra complexity. I suppose the scenario to worry about is that since we're holding the mutex while we call async->send we could cause the background to wake up and immediately be blocked on the mutex... but it doesn't seem like it would be that common or that costly when it did happen?

/cc @tmpsantos @jfirebaugh @kkaefer @ivovandongen

[ChrisLoer]

I audited the code of our four implementations of AsyncTask and I don't see any way this could cause a deadlock.

😅 My in-depth audit missed that the Android RunLoop doesn't use AsyncTask... The Android 'wake' looks like write(fds[PIPE_IN], "\n", 1)... which is... I dunno, is it OK to hold a mutex while writing to a file descriptor?

[kkaefer]

Now that we lock the entire function, we can also remove the withMutex function altogether and just move the std::lock_guard into every function.

[tmpsantos]

👍 on @kkaefer point here

[ChrisLoer]

withMutex is also used in process, and I think there's a readability win in using the same syntax to mark all locked sections in the code. If we were to stop using withMutex in push, I think it would also make sense to change the call in process to something like:

...
{
  std::lock_guard<std::mutex> lock(mutex);
  queue_.swap(queue);
}
...

Between those two options... 🤷‍♂️ ? Doesn't make a big difference to me, but at some point we thought withMutex was a clearer way to mark the locked section of code.

[tmpsantos]

is it OK to hold a mutex while writing to a file descriptor?

Should be fine.

[tmpsantos]

@ChrisLoer thanks! This apparently fixed a hang I could only reproduce on Qt + Windows.