1
0
mirror of https://github.com/qTox/qTox.git synced 2024-03-22 14:00:36 +08:00

Fix theoritical A/V deadlock

Could only be hit by pausing at a key point in a debugger until the call timed-out.

Having one thread going up the call stack and acquiring locks (toxcore callbacks), while another thread goes down taking locks in the other order (CoreAV calling toxav functions) creates some pretty freezy situations.

The deadlock was caused by the GUI thread calling the CoreAV thread, acquiring the CoreAV callback, then right before calling a toxav function, not schedule the thread until the call times out. At this point the toxcore thread fires its state callback to tell us the call is over, locking internal toxcore/toxav mutexes, it reaches our callback function which tries to switch to the CoreAV thread to clean up the call data structures, but has to wait since the CoreAV thread holds its own lock. At this point if we resume the CoreAV thread, it'll be busy calling into a toxav function, which tries to acquire internal toxav locks, those locks are held by the toxcore callback so we deadlock.

Our solution is that when getting a toxcore callback, we immediately switch to a temporary thread, allowing toxcore to release the locks it held, and that temporary thread tries to switch to do work on call data structures. Meanwhile if the CoreAV thread needs internal toxcore locks, it can get them.
This commit is contained in:
tux3 2015-11-08 00:32:32 +01:00
parent cf52ff314f
commit 523e419adf
No known key found for this signature in database
GPG Key ID: 7E086DD661263264

View File

@ -29,6 +29,7 @@
#include <QTimer> #include <QTimer>
#include <QDebug> #include <QDebug>
#include <QCoreApplication> #include <QCoreApplication>
#include <QtConcurrent/QtConcurrentRun>
#ifdef QTOX_FILTER_AUDIO #ifdef QTOX_FILTER_AUDIO
#include "src/audio/audiofilterer.h" #include "src/audio/audiofilterer.h"
@ -468,21 +469,26 @@ void CoreAV::callCallback(ToxAV* toxav, uint32_t friendNum, bool audio, bool vid
{ {
CoreAV* self = static_cast<CoreAV*>(_self); CoreAV* self = static_cast<CoreAV*>(_self);
// Run this slow path callback asynchronously on the AV thread to avoid deadlocks // Run this slow callback asynchronously on the AV thread to avoid deadlocks with what our caller (toxcore) holds
// Also run the code to switch to the CoreAV thread in yet another thread, in case CoreAV
// has threadSwitchLock and wants a toxcore lock that our call stack is holding...
if (QThread::currentThread() != self->coreavThread.get()) if (QThread::currentThread() != self->coreavThread.get())
{ {
QtConcurrent::run([=](){
// We assume the original caller doesn't come from the CoreAV thread here // We assume the original caller doesn't come from the CoreAV thread here
while (self->threadSwitchLock.test_and_set(std::memory_order_acquire)) while (self->threadSwitchLock.test_and_set(std::memory_order_acquire))
QThread::yieldCurrentThread(); // Shouldn't spin for long, we have priority QThread::yieldCurrentThread(); // Shouldn't spin for long, we have priority
return (void)QMetaObject::invokeMethod(self, "callCallback", Qt::QueuedConnection, QMetaObject::invokeMethod(self, "callCallback", Qt::QueuedConnection,
Q_ARG(ToxAV*, toxav), Q_ARG(uint32_t, friendNum), Q_ARG(ToxAV*, toxav), Q_ARG(uint32_t, friendNum),
Q_ARG(bool, audio), Q_ARG(bool, video), Q_ARG(void*, _self)); Q_ARG(bool, audio), Q_ARG(bool, video), Q_ARG(void*, _self));
});
return;
} }
if (self->calls.contains(friendNum)) if (self->calls.contains(friendNum))
{ {
/// NOTE: Hanging up from a callback is supposed to be UB, /// Hanging up from a callback is supposed to be UB,
/// but since currently the toxav callbacks are fired from the toxcore thread, /// but since currently the toxav callbacks are fired from the toxcore thread,
/// we'll always reach this point through a non-blocking queud connection, so not in the callback. /// we'll always reach this point through a non-blocking queud connection, so not in the callback.
qWarning() << QString("Rejecting call invite from %1, we're already in that call!").arg(friendNum); qWarning() << QString("Rejecting call invite from %1, we're already in that call!").arg(friendNum);
@ -508,15 +514,21 @@ void CoreAV::stateCallback(ToxAV* toxav, uint32_t friendNum, uint32_t state, voi
{ {
CoreAV* self = static_cast<CoreAV*>(_self); CoreAV* self = static_cast<CoreAV*>(_self);
// Run this slow path callback asynchronously on the AV thread to avoid deadlocks // Run this slow callback asynchronously on the AV thread to avoid deadlocks with what our caller (toxcore) holds
// Also run the code to switch to the CoreAV thread in yet another thread, in case CoreAV
// has threadSwitchLock and wants a toxcore lock that our call stack is holding...
if (QThread::currentThread() != self->coreavThread.get()) if (QThread::currentThread() != self->coreavThread.get())
{ {
QtConcurrent::run([=](){
// We assume the original caller doesn't come from the CoreAV thread here // We assume the original caller doesn't come from the CoreAV thread here
while (self->threadSwitchLock.test_and_set(std::memory_order_acquire)) while (self->threadSwitchLock.test_and_set(std::memory_order_acquire))
QThread::yieldCurrentThread(); // Shouldn't spin for long, we have priority QThread::yieldCurrentThread(); // Shouldn't spin for long, we have priority
return (void)QMetaObject::invokeMethod(self, "stateCallback", Qt::QueuedConnection,
QMetaObject::invokeMethod(self, "stateCallback", Qt::QueuedConnection,
Q_ARG(ToxAV*, toxav), Q_ARG(uint32_t, friendNum), Q_ARG(ToxAV*, toxav), Q_ARG(uint32_t, friendNum),
Q_ARG(uint32_t, state), Q_ARG(void*, _self)); Q_ARG(uint32_t, state), Q_ARG(void*, _self));
});
return;
} }
if(!self->calls.contains(friendNum)) if(!self->calls.contains(friendNum))