From 589ff6163311af9b6ee5655fbac80a9cae57823e Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Wed, 7 Apr 2021 21:52:49 +0200 Subject: [PATCH 1/9] Recorder: Gracefully handle startup failures Failures to create required directories or files had been handled via exceptions already. However, these exceptions were not caught anywhere. This made them end up in Qt's default exception handler which crashed the whole application. This commit adds proper exception handling, logging and propagation for the recorder failure state back to the jam controller. This fixes crashes due to lack of permission, lack of disk space or other file system errors. Fixes #1289 --- src/recorder/jamcontroller.cpp | 16 ++++++++++++++++ src/recorder/jamcontroller.h | 2 ++ src/recorder/jamrecorder.cpp | 26 ++++++++++++++++++++++++-- src/recorder/jamrecorder.h | 1 + 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index 164f6a43fd..a7168ebb95 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -160,6 +160,10 @@ void CJamController::SetRecordingDir ( QString newRecordingDir, QObject::connect ( pJamRecorder, &CJamRecorder::RecordingSessionStarted, this, &CJamController::RecordingSessionStarted ); + // from the recorder to the controller + QObject::connect ( pJamRecorder, &CJamRecorder::RecordingFailed, + this, &CJamController::OnRecordingFailed ); + pthJamRecorder->start ( QThread::NormalPriority ); } @@ -169,6 +173,18 @@ void CJamController::SetRecordingDir ( QString newRecordingDir, } } +void CJamController::OnRecordingFailed ( QString error ) +{ + if ( !bEnableRecording ) + { + // Recording has already been stopped, possibly + // by a previous OnRecordingFailed call from another client/thread. + return; + } + qWarning() << "Could not start recording:" << error; + SetEnableRecording ( false, true ); +} + ERecorderState CJamController::GetRecorderState() { // return recorder state diff --git a/src/recorder/jamcontroller.h b/src/recorder/jamcontroller.h index 82b714f430..044d958d1d 100755 --- a/src/recorder/jamcontroller.h +++ b/src/recorder/jamcontroller.h @@ -46,6 +46,8 @@ class CJamController : public QObject ERecorderState GetRecorderState(); private: + void OnRecordingFailed ( QString error ); + CServer* pServer; bool bRecorderInitialised; diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 07653bb469..398a98d35a 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -396,14 +396,30 @@ void CJamRecorder::Start() { // Ensure any previous cleaning up has been done. OnEnd(); + QString error; + // needs to be after OnEnd() as that also locks ChIdMutex.lock(); { - currentSession = new CJamSession( recordBaseDir ); - isRecording = true; + try + { + currentSession = new CJamSession( recordBaseDir ); + isRecording = true; + } + catch ( const std::runtime_error& err ) + { + currentSession = nullptr; + error = err.what(); + } } ChIdMutex.unlock(); + if ( !currentSession ) + { + emit RecordingFailed ( error ); + return; + } + emit RecordingSessionStarted ( currentSession->SessionDir().path() ); } @@ -585,6 +601,12 @@ void CJamRecorder::OnFrame(const int iChID, const QString name, const CHostAddre Start(); } + // Start() may have failed, so check again: + if ( !isRecording ) + { + return; + } + // needs to be after Start() as that also locks ChIdMutex.lock(); { diff --git a/src/recorder/jamrecorder.h b/src/recorder/jamrecorder.h index 930b8e551f..7c53662f8f 100755 --- a/src/recorder/jamrecorder.h +++ b/src/recorder/jamrecorder.h @@ -178,6 +178,7 @@ class CJamRecorder : public QObject signals: void RecordingSessionStarted ( QString sessionDir ); + void RecordingFailed ( QString error ); public slots: /** From 7ad8248a9781b646344ee5fdf19e0cbdaa7e55fb Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Wed, 7 Apr 2021 22:05:51 +0200 Subject: [PATCH 2/9] Recorder: Improve error message --- src/recorder/jamrecorder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 398a98d35a..06459e9e3f 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -160,7 +160,7 @@ CJamSession::CJamSession(QDir recordBaseDir) : if (!fi.exists() && !QDir().mkpath(sessionDir.absolutePath())) { - throw std::runtime_error( (sessionDir.absolutePath() + " does not exist but could not be created").toStdString() ); + throw std::runtime_error( (sessionDir.absolutePath() + " does not exist and could not be created").toStdString() ); } if (!fi.isDir()) { From c3e64429c50363be50033937373f6be66519b065 Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Wed, 7 Apr 2021 22:25:19 +0200 Subject: [PATCH 3/9] Recorder: Clarify --norecord log output Fixes #1284 Co-authored-by: Peter L Jones --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 80112a3f0f..1cf578d10f 100755 --- a/src/main.cpp +++ b/src/main.cpp @@ -397,7 +397,7 @@ int main ( int argc, char** argv ) "--norecord" ) ) { bDisableRecording = true; - qInfo() << "- recording will not be enabled"; + qInfo() << "- recording will not take place until enabled"; CommandLineOptions << "--norecord"; continue; } From 41dcb48c610bb566ac6e3af32a9151bfa30c1763 Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Thu, 8 Apr 2021 21:52:02 +0200 Subject: [PATCH 4/9] Recorder: document SessionDirToReaper's use --- src/recorder/jamrecorder.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 06459e9e3f..74b2e90195 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -532,6 +532,8 @@ void CJamRecorder::AudacityLofFromCurrentSession() /** * @brief CJamRecorder::SessionDirToReaper Replica of CJamRecorder::OnEnd() but using the directory contents to construct the CReaperProject object * @param strSessionDirName + * + * This is used for testing and is not called from the regular Jamulus code. */ void CJamRecorder::SessionDirToReaper(QString& strSessionDirName, int serverFrameSizeSamples) { From 9659682a27dc6701e670b0dd6dc303ccfd660c3a Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Mon, 12 Apr 2021 17:17:13 +0100 Subject: [PATCH 5/9] Tell the server to disable recording on failure. --- src/recorder/jamcontroller.cpp | 8 ++++++-- src/recorder/jamcontroller.h | 9 +++++++-- src/server.cpp | 1 + 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index a7168ebb95..13ff36fe17 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -26,7 +26,8 @@ using namespace recorder; -CJamController::CJamController() : +CJamController::CJamController( CServer* pNServer ) : + pServer ( pNServer ), bRecorderInitialised ( false ), bEnableRecording ( false ), strRecordingDir ( "" ), @@ -182,7 +183,10 @@ void CJamController::OnRecordingFailed ( QString error ) return; } qWarning() << "Could not start recording:" << error; - SetEnableRecording ( false, true ); + // Turn off recording until it is manually re-enabled via UI or signals. + // This needs to be done from the CServer side to cover all relevant + // state. + pServer->SetEnableRecording ( false ); } ERecorderState CJamController::GetRecorderState() diff --git a/src/recorder/jamcontroller.h b/src/recorder/jamcontroller.h index 044d958d1d..68fa098253 100755 --- a/src/recorder/jamcontroller.h +++ b/src/recorder/jamcontroller.h @@ -34,7 +34,7 @@ class CJamController : public QObject { Q_OBJECT public: - explicit CJamController(); + explicit CJamController( CServer* pNServer ); bool GetRecorderInitialised() { return bRecorderInitialised; } QString GetRecorderErrMsg() { return strRecorderErrMsg; } @@ -48,7 +48,7 @@ class CJamController : public QObject private: void OnRecordingFailed ( QString error ); - CServer* pServer; + CServer* pServer; bool bRecorderInitialised; bool bEnableRecording; @@ -75,4 +75,9 @@ class CJamController : public QObject } +// This must be included AFTER the above definition of class CJamController, +// because server.h defines CServer, which contains an instance of CJamController, +// and therefore needs to know the its size. +#include "../server.h" + Q_DECLARE_METATYPE(int16_t) diff --git a/src/server.cpp b/src/server.cpp index 99c7ab5c48..c2f553a810 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -255,6 +255,7 @@ CServer::CServer ( const int iNewMaxNumChan, strServerListFilter, iNewMaxNumChan, &ConnLessProtocol ), + JamController ( this ), bDisableRecording ( bDisableRecording ), bAutoRunMinimized ( false ), bDelayPan ( bNDelayPan ), From 7e7a4e86cc39d912e8577f3c3f8dd89f5ccb001f Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Mon, 12 Apr 2021 23:10:18 +0200 Subject: [PATCH 6/9] Server: Show recorder failures in UI --- src/recorder/jamcontroller.cpp | 1 + src/serverdlg.cpp | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/src/recorder/jamcontroller.cpp b/src/recorder/jamcontroller.cpp index 13ff36fe17..5a2fc82e0c 100755 --- a/src/recorder/jamcontroller.cpp +++ b/src/recorder/jamcontroller.cpp @@ -182,6 +182,7 @@ void CJamController::OnRecordingFailed ( QString error ) // by a previous OnRecordingFailed call from another client/thread. return; } + strRecorderErrMsg = error; qWarning() << "Could not start recording:" << error; // Turn off recording until it is manually re-enabled via UI or signals. // This needs to be done from the CServer side to cover all relevant diff --git a/src/serverdlg.cpp b/src/serverdlg.cpp index fdf6c9a824..170d41a095 100644 --- a/src/serverdlg.cpp +++ b/src/serverdlg.cpp @@ -600,6 +600,12 @@ void CServerDlg::OnServerStopped() void CServerDlg::OnStopRecorder() { UpdateRecorderStatus ( QString::null ); + if ( pServer->GetRecorderErrMsg() != QString::null ) + { + QMessageBox::warning ( this, APP_NAME, tr ( "Recorder failed to start. " + "Please check available disk space and permissions and try again. " + "Error: " ) + pServer->GetRecorderErrMsg() ); + } } void CServerDlg::OnRecordingDirClicked() From b8eb35290e67607c55becfbb309874e3168d2fef Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Mon, 12 Apr 2021 23:12:25 +0200 Subject: [PATCH 7/9] Server: Update recorder checkbox state automatically --- src/serverdlg.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/serverdlg.cpp b/src/serverdlg.cpp index 170d41a095..716b5922fb 100644 --- a/src/serverdlg.cpp +++ b/src/serverdlg.cpp @@ -849,6 +849,10 @@ void CServerDlg::UpdateRecorderStatus ( QString sessionDir ) strRecorderStatus = SREC_NOT_INITIALISED; } + chbEnableRecorder->blockSignals ( true ); + chbEnableRecorder->setChecked ( strRecorderStatus != SREC_NOT_ENABLED ); + chbEnableRecorder->blockSignals ( false ); + edtRecordingDir->setText ( strRecordingDir ); edtCurrentSessionDir->setEnabled ( bIsRecording ); lblRecorderStatus->setText ( strRecorderStatus ); From 2cbe9facd6e96646054dcd6b3f8083a01dfb9dba Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Mon, 12 Apr 2021 23:46:03 +0200 Subject: [PATCH 8/9] Server: Disallow recording directory change during active recording --- src/serverdlg.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/serverdlg.cpp b/src/serverdlg.cpp index 716b5922fb..5f031faf9e 100644 --- a/src/serverdlg.cpp +++ b/src/serverdlg.cpp @@ -854,6 +854,9 @@ void CServerDlg::UpdateRecorderStatus ( QString sessionDir ) chbEnableRecorder->blockSignals ( false ); edtRecordingDir->setText ( strRecordingDir ); + edtRecordingDir->setEnabled ( !bIsRecording ); + pbtRecordingDir->setEnabled ( !bIsRecording ); + tbtClearRecordingDir->setEnabled ( !bIsRecording ); edtCurrentSessionDir->setEnabled ( bIsRecording ); lblRecorderStatus->setText ( strRecorderStatus ); pbtNewRecording->setEnabled ( bIsRecording ); From 353e75619af6db9ee39dfc58441fb9f774d7d590 Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Fri, 30 Apr 2021 23:24:36 +0200 Subject: [PATCH 9/9] Recorder: Use CGenErr instead of runtime_error --- src/recorder/jamrecorder.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/recorder/jamrecorder.cpp b/src/recorder/jamrecorder.cpp index 74b2e90195..3ba04f51c0 100755 --- a/src/recorder/jamrecorder.cpp +++ b/src/recorder/jamrecorder.cpp @@ -60,7 +60,7 @@ CJamClient::CJamClient(const qint64 frame, const int _numChannels, const QString wavFile = new QFile(recordBaseDir.absoluteFilePath(fileName)); if (!wavFile->open(QFile::OpenMode(QIODevice::OpenModeFlag::ReadWrite))) // need to allow rewriting headers { - throw new std::runtime_error( ("Could not write to WAV file " + wavFile->fileName()).toStdString() ); + throw CGenErr ( "Could not write to WAV file " + wavFile->fileName() ); } out = new CWaveStream(wavFile, numChannels); @@ -160,15 +160,15 @@ CJamSession::CJamSession(QDir recordBaseDir) : if (!fi.exists() && !QDir().mkpath(sessionDir.absolutePath())) { - throw std::runtime_error( (sessionDir.absolutePath() + " does not exist and could not be created").toStdString() ); + throw CGenErr ( sessionDir.absolutePath() + " does not exist and could not be created" ); } if (!fi.isDir()) { - throw std::runtime_error( (sessionDir.absolutePath() + " exists but is not a directory").toStdString() ); + throw CGenErr ( sessionDir.absolutePath() + " exists but is not a directory" ); } if (!fi.isWritable()) { - throw std::runtime_error( (sessionDir.absolutePath() + " is a directory but cannot be written to").toStdString() ); + throw CGenErr ( sessionDir.absolutePath() + " is a directory but cannot be written to" ); } // Explicitly set all the pointers to "empty" @@ -406,10 +406,10 @@ void CJamRecorder::Start() { currentSession = new CJamSession( recordBaseDir ); isRecording = true; } - catch ( const std::runtime_error& err ) + catch ( const CGenErr& err ) { currentSession = nullptr; - error = err.what(); + error = err.GetErrorText(); } } ChIdMutex.unlock(); @@ -540,7 +540,7 @@ void CJamRecorder::SessionDirToReaper(QString& strSessionDirName, int serverFram const QFileInfo fiSessionDir(QDir::cleanPath(strSessionDirName)); if (!fiSessionDir.exists() || !fiSessionDir.isDir()) { - throw std::runtime_error( (fiSessionDir.absoluteFilePath() + " does not exist or is not a directory. Aborting.").toStdString() ); + throw CGenErr ( fiSessionDir.absoluteFilePath() + " does not exist or is not a directory. Aborting." ); } const QDir dSessionDir(fiSessionDir.absoluteFilePath()); @@ -548,12 +548,13 @@ void CJamRecorder::SessionDirToReaper(QString& strSessionDirName, int serverFram const QFileInfo fiRPP(reaperProjectFileName); if (fiRPP.exists()) { - throw std::runtime_error( (fiRPP.absoluteFilePath() + " exists and will not be overwritten. Aborting.").toStdString() ); + throw CGenErr ( fiRPP.absoluteFilePath() + " exists and will not be overwritten. Aborting." ); } QFile outf (fiRPP.absoluteFilePath()); - if (!outf.open(QFile::WriteOnly)) { - throw std::runtime_error( (fiRPP.absoluteFilePath() + " could not be written. Aborting.").toStdString() ); + if ( !outf.open ( QFile::WriteOnly ) ) + { + throw CGenErr ( fiRPP.absoluteFilePath() + " could not be written. Aborting." ); } QTextStream out(&outf);