Fix memory leak and race condition relating to DSP*Engines. Part of f…

…4exb#2159
srcejon · Nov 14, 2024 · 6dcc342 · 6dcc342
1 parent 7c7a7d7
commit 6dcc342
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 17 deletions.
diff --git a/sdrbase/dsp/dspengine.cpp b/sdrbase/dsp/dspengine.cpp
@@ -187,39 +187,41 @@ void DSPEngine::removeLastDeviceMIMOEngine()
     }
 }
 
-QThread * DSPEngine::removeDeviceEngineAt(int deviceIndex)
+QThread *DSPEngine::getDeviceEngineThread(int deviceIndex)
 {
     if (deviceIndex >= m_deviceEngineReferences.size()) {
         return nullptr;
+    } else {
+        return m_deviceEngineReferences[deviceIndex].m_thread;
     }
+}
 
-    QThread *deviceThread = nullptr;
+void DSPEngine::removeDeviceEngineAt(int deviceIndex)
+{
+    if (deviceIndex >= m_deviceEngineReferences.size()) {
+        return;
+    }
 
     if (m_deviceEngineReferences[deviceIndex].m_deviceEngineType == 0) // source
     {
         DSPDeviceSourceEngine *deviceEngine = m_deviceEngineReferences[deviceIndex].m_deviceSourceEngine;
-        deviceThread = m_deviceEngineReferences[deviceIndex].m_thread;
-        deviceThread->exit();
+        m_deviceEngineReferences[deviceIndex].m_thread->exit();
         m_deviceSourceEngines.removeAll(deviceEngine);
     }
     else if (m_deviceEngineReferences[deviceIndex].m_deviceEngineType == 1) // sink
     {
         DSPDeviceSinkEngine *deviceEngine = m_deviceEngineReferences[deviceIndex].m_deviceSinkEngine;
-        deviceThread = m_deviceEngineReferences[deviceIndex].m_thread;
-        deviceThread->exit();
+        m_deviceEngineReferences[deviceIndex].m_thread->exit();
         m_deviceSinkEngines.removeAll(deviceEngine);
     }
     else if (m_deviceEngineReferences[deviceIndex].m_deviceEngineType == 2) // MIMO
     {
         DSPDeviceMIMOEngine *deviceEngine = m_deviceEngineReferences[deviceIndex].m_deviceMIMOEngine;
-        deviceThread = m_deviceEngineReferences[deviceIndex].m_thread;
-        deviceThread->exit();
+        m_deviceEngineReferences[deviceIndex].m_thread->exit();
         m_deviceMIMOEngines.removeAll(deviceEngine);
     }
 
     m_deviceEngineReferences.removeAt(deviceIndex);
-
-    return deviceThread;
 }
 
 void DSPEngine::createFFTFactory(const QString& fftWisdomFileName)

diff --git a/sdrbase/dsp/dspengine.h b/sdrbase/dsp/dspengine.h
@@ -53,7 +53,8 @@ class SDRBASE_API DSPEngine : public QObject {
 	DSPDeviceMIMOEngine *addDeviceMIMOEngine();
 	void removeLastDeviceMIMOEngine();
 
-    QThread *removeDeviceEngineAt(int deviceIndex);
+    QThread *getDeviceEngineThread(int deviceIndex);
+    void removeDeviceEngineAt(int deviceIndex);
 
 	AudioDeviceManager *getAudioDeviceManager() { return &m_audioDeviceManager; }
 

diff --git a/sdrgui/mainwindow.cpp b/sdrgui/mainwindow.cpp
@@ -778,10 +778,19 @@ void RemoveDeviceSetFSM::removeUI()
 void RemoveDeviceSetFSM::stopEngine()
 {
     qDebug() << "RemoveDeviceSetFSM::stopEngine";
-    QThread *thread = m_mainWindow->m_dspEngine->removeDeviceEngineAt(m_deviceSetIndex);
-    if (thread && !thread->isFinished())    // FIXME: Is there a race condition here? We might need to connect before calling thread->exit
+    QThread *thread = m_mainWindow->m_dspEngine->getDeviceEngineThread(m_deviceSetIndex);
+
+    if (thread)
     {
-        connect(thread, &QThread::finished, m_mainWindow, &MainWindow::engineStopped);
+        bool finished = thread->isFinished();
+
+        if (!finished) {
+            connect(thread, &QThread::finished, m_mainWindow, &MainWindow::engineStopped);
+        }
+        m_mainWindow->m_dspEngine->removeDeviceEngineAt(m_deviceSetIndex);
+        if (finished) {
+            emit m_mainWindow->engineStopped();
+        }
     }
     else
     {
@@ -801,12 +810,20 @@ void RemoveDeviceSetFSM::removeDeviceSet()
 
     DeviceAPI *deviceAPI = m_deviceUISet->m_deviceAPI;
     delete m_deviceUISet;
-    if (m_deviceSourceEngine) {
+    if (m_deviceSourceEngine)
+    {
         delete deviceAPI->getSampleSource();
-    } else if (m_deviceSinkEngine) {
+        delete m_deviceSourceEngine;
+    }
+    else if (m_deviceSinkEngine)
+    {
         delete deviceAPI->getSampleSink();
-    } else {
+        delete m_deviceSinkEngine;
+    }
+    else
+    {
         delete deviceAPI->getSampleMIMO();
+        delete m_deviceMIMOEngine;
     }
     delete deviceAPI;