View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0017319 | Scribus | Import / Export | public | 2024-11-22 14:16 | 2025-06-07 07:26 |
Reporter | tropion | Assigned To | |||
Priority | normal | Severity | minor | Reproducibility | sometimes |
Status | new | Resolution | open | ||
Product Version | 1.7.0.svn | ||||
Summary | 0017319: [PATCH] Scribuses working in parallel overwrite each others temp files | ||||
Description | I tried to use several scribuses in parallel to convert .sla files to .pdf files. Sometimes the resulting .pdfs contain corrupt images. This happens, because the different scribuses all use the same temp file names and sometimes they overwrite each others temp files. I suggest adding something like QString::number(QCoreApplication::applicationPid()) as a subdir to the path in ScPaths::tempFileDir(). And though I am really not sure if this is the right place to clean that up I suggest adding QDir tmpDir (ScPaths::tempFileDir()); tmpDir.rmdir (ScPaths::tempFileDir()); to ScribusQApp::~ScribusQApp() | ||||
Tags | No tags attached. | ||||
Patch | Yes | ||||
|
tempDir.diff (1,334 bytes)
diff --git a/scribus/scpaths.cpp b/scribus/scpaths.cpp index 74b8fdf14..d17fb3e0f 100644 --- a/scribus/scpaths.cpp +++ b/scribus/scpaths.cpp @@ -656,11 +656,16 @@ QString ScPaths::tempFileDir() // GetTempPath may return Windows directory, better not use this one // for temporary files if (QDir(tempPath).exists() && tempPath != windowsSpecialDir(CSIDL_WINDOWS)) - return tempPath; + { + QDir tempAppDirectory(tempPath + QString::number(QCoreApplication::applicationPid())); + if (!tempAppDirectory.exists()) + tempAppDirectory.mkpath(tempAppDirectory.absolutePath()); + return tempAppDirectory.absolutePath() + "/"; + } } #endif - QDir tempAppDirectory(applicationDataDir() + "temp/"); + QDir tempAppDirectory(applicationDataDir() + "temp/" + QString::number(QCoreApplication::applicationPid())); if (!tempAppDirectory.exists()) tempAppDirectory.mkpath(tempAppDirectory.absolutePath()); return tempAppDirectory.absolutePath() + "/"; diff --git a/scribus/scribusapp.cpp b/scribus/scribusapp.cpp index 159ac2f0e..330e1c993 100644 --- a/scribus/scribusapp.cpp +++ b/scribus/scribusapp.cpp @@ -110,6 +110,8 @@ ScribusQApp::~ScribusQApp() { delete m_ScCore; delete m_scDLMgr; + QDir tmpDir (ScPaths::tempFileDir()); + tmpDir.rmdir (ScPaths::tempFileDir()); LanguageManager::deleteInstance(); } |
|
i guess that we would need some code to clean up the old directory that have been left around by scribuses that have crashed... |
|
We could add a lock file to the tempFileDir. That lock file would be accessible only once the corresponding Scribus has crashed. We could then delete all "temp\Scribus-*" folders with an accessible lock file in e.g. ~ScribusQApp(). |
|
I wonder if Scribus shouldn't use https://doc.qt.io/qt-6/qtemporarydir.html ... Or are there good reasons for avoiding it? |
|
I see no reason to avoid QTemporaryDir. I just didn't realise that we had such nice things available. |
|
would you mind integrating the newly acquired knowledge into your patch and check if it fits your needs? |
|
This new patch uses QTemporaryDir and it cleans up the temporary directories left behind by crashed Scribuses. tempDir-withCleanup.diff (5,375 bytes)
diff --git a/scribus/scpaths.cpp b/scribus/scpaths.cpp index 18401c76d..076aaa85f 100644 --- a/scribus/scpaths.cpp +++ b/scribus/scpaths.cpp @@ -10,6 +10,7 @@ for which a new license (GPL+exception) is in place. #include <QDir> #include <QProcess> #include <QStandardPaths> +#include <QLockFile> #include "prefsmanager.h" #include "scconfig.h" @@ -33,6 +34,13 @@ const char ScPaths::envPathSeparator = ';'; const char ScPaths::envPathSeparator = ':'; #endif +// ScPaths::tempDir and the things needed for cleanupTempDir +const QString ScPaths::lockFilename = "Scribus.lock"; +const QString ScPaths::tempDirPrefix = "Scribus-temp-"; +const QTemporaryDir ScPaths::tempDir { QTemporaryDir (tempDirPrefix + "XXXXXX") }; +QLockFile ScPaths::lockFile{ tempDir.path() + "/" + lockFilename }; +char ScPaths::lockFileInitializer {ScPaths::doLockFile()}; + // Init the singleton's "self" address to nullptr ScPaths* ScPaths::m_instance = nullptr; @@ -664,26 +672,52 @@ QString ScPaths::scrapbookDir(bool createIfNotExists) QString ScPaths::tempFileDir() { -#ifdef Q_OS_WIN32 - QString tempPath; - WCHAR wTempPath[1024]; - DWORD result = GetTempPathW(1024, wTempPath); - if (result) + return tempDir.path() + '/'; +} +/* + Make sure the lockfile is locked from the very beginning. + The lockfile is locked as long as Scribus is running. + It is used in cleanupTemp() to distinguish between tempDirs left + behind by crashed Scribuses (unlocked) and tempDirs that are still in use (locked). +*/ +char ScPaths::doLockFile() +{ + lockFile.setStaleLockTime(0); // Lock as long as this Scribus runs. + return ScPaths::lockFile.tryLock(500) ? '1' : '0'; // The values are never used. + // This is just to lock the file as early as possible +}; // so no other Scribus cleans up this Scribuses tempDir + + +/* + cleanup temporary directories leftover by crashed Scribuses. +*/ +void ScPaths::cleanupTemp() + +{ + QDir dir(tempDir.path()); + dir.cdUp(); + QStringList searchFor(tempDirPrefix + "*"); + QString dbg1(dir.absolutePath()); + + QStringList entries(dir.entryList(searchFor)); + for (QString entry : entries) { - tempPath = QString::fromUtf16((const unsigned short*) wTempPath); - tempPath.replace('\\', '/'); - tempPath += "/"; - // GetTempPath may return Windows directory, better not use this one - // for temporary files - if (QDir(tempPath).exists() && tempPath != windowsSpecialDir(CSIDL_WINDOWS)) - return tempPath; + QLockFile lockTester(dir.absolutePath() + "/" + entry + "/" + lockFilename); + lockTester.setStaleLockTime(500); // 500ms seems really long but + // network storages should to be able to answer in time + // if this times out because the answer takes too long + // unused temp dirs will not get cleaned up + // and shutting down Scribus will take long + if (lockTester.tryLock()) + { + // The lock is aquired. + // This means the Scribus that once used the temporary + // directory containing the lockfile is no longer running. + lockTester.unlock(); + QDir deleteThis(dir.absolutePath() + "/" + entry); + deleteThis.removeRecursively(); + } } -#endif - - QDir tempAppDirectory(applicationDataDir() + "temp/"); - if (!tempAppDirectory.exists()) - tempAppDirectory.mkpath(tempAppDirectory.absolutePath()); - return tempAppDirectory.absolutePath() + "/"; } QString ScPaths::downloadDir() diff --git a/scribus/scpaths.h b/scribus/scpaths.h index e4f1f918b..b5334b4da 100644 --- a/scribus/scpaths.h +++ b/scribus/scpaths.h @@ -9,6 +9,8 @@ for which a new license (GPL+exception) is in place. #include <QString> #include <QStringList> +#include <QTemporaryDir> +#include <QLockFile> #include "scribusapi.h" class SCRIBUS_API ScPaths @@ -31,6 +33,20 @@ public: /*** The separator used between path list entries in environment variables */ static const char envPathSeparator; + + /* */ + static const QString lockFilename; + static const QString tempDirPrefix; + + /*** The temporary directory is used for temporary files */ + static const QTemporaryDir tempDir; + + /*** The lockFile is used to tell if tempDir is in use */ + static QLockFile lockFile; + /*** The initializer makes sure that the file is locked as early as possible */ + static char lockFileInitializer; + static char doLockFile(); + /** @brief Return path to documentation directory */ const QString& docDir() const; /** @brief Return path to icons directory*/ @@ -95,6 +111,8 @@ public: static QString scrapbookDir(bool createIfNotExists); /** @brief Return path to directory used for temporary files*/ static QString tempFileDir(); + /** @brief to cleanup temporary directories leftover by crashed Scribuses*/ + static void cleanupTemp(); /** @brief Return path to directory used for downloaded (permanent) files*/ static QString downloadDir(); /** @brief Return path to Contents OSX subdirectory*/ diff --git a/scribus/scribusapp.cpp b/scribus/scribusapp.cpp index db7ae2903..d61b67d29 100644 --- a/scribus/scribusapp.cpp +++ b/scribus/scribusapp.cpp @@ -107,6 +107,7 @@ ScribusQApp::~ScribusQApp() { delete m_ScCore; delete m_scDLMgr; + ScPaths::cleanupTemp(); LanguageManager::deleteInstance(); } |
|
a first quick comment: i'd prefer the comments not been placed on the same line as the code. normally, it should be enough to add a doc comment before the method declaration (in the .h) file. and then use telling function and variable names and avoid commenting every single line. of course, sometimes a comment in the body is needed. up to you to judge if it's the case in your code. but, please, would you mind putting the comments before the code it documents, not at the end of the line? i will try to further review the code "later" .... |
|
I tried again. tempDir-3.diff (4,542 bytes)
diff --git a/scribus/scpaths.h b/scribus/scpaths.h index e4f1f918b..90375f6bd 100644 --- a/scribus/scpaths.h +++ b/scribus/scpaths.h @@ -9,6 +9,8 @@ for which a new license (GPL+exception) is in place. #include <QString> #include <QStringList> +#include <QTemporaryDir> +#include <QLockFile> #include "scribusapi.h" class SCRIBUS_API ScPaths @@ -95,6 +97,8 @@ public: static QString scrapbookDir(bool createIfNotExists); /** @brief Return path to directory used for temporary files*/ static QString tempFileDir(); + /** @brief Cleanup temporary directories left behind by crashed Scribuses*/ + static void cleanupTemp(); /** @brief Return path to directory used for downloaded (permanent) files*/ static QString downloadDir(); /** @brief Return path to Contents OSX subdirectory*/ @@ -116,6 +120,18 @@ protected: /** @brief Pointer to existing instance of ScPaths, if any. */ static ScPaths* m_instance; + /*** The lockFile is used to tell if tempDir is in use */ + static QLockFile lockFile; + static const QString lockFilename; + + /*** The initializer makes sure that the file is locked as early as possible */ + static char lockFileInitializer; + static char doLockFile(); + + /*** The temporary directory is used for temporary files */ + static const QTemporaryDir tempDir; + static const QString tempDirPrefix; + // Members to hold system paths QString m_docDir; QString m_fontDir; diff --git a/scribus/scpaths.cpp b/scribus/scpaths.cpp index 18401c76d..d7cfbf7bb 100644 --- a/scribus/scpaths.cpp +++ b/scribus/scpaths.cpp @@ -10,6 +10,7 @@ for which a new license (GPL+exception) is in place. #include <QDir> #include <QProcess> #include <QStandardPaths> +#include <QLockFile> #include "prefsmanager.h" #include "scconfig.h" @@ -33,6 +34,13 @@ const char ScPaths::envPathSeparator = ';'; const char ScPaths::envPathSeparator = ':'; #endif +// ScPaths::tempDir and the things needed for cleanupTempDir +const QString ScPaths::lockFilename = "Scribus.lock"; +const QString ScPaths::tempDirPrefix = "Scribus-temp-"; +const QTemporaryDir ScPaths::tempDir { QTemporaryDir (tempDirPrefix + "XXXXXX") }; +QLockFile ScPaths::lockFile{ tempDir.path() + "/" + lockFilename }; +char ScPaths::lockFileInitializer {ScPaths::doLockFile()}; + // Init the singleton's "self" address to nullptr ScPaths* ScPaths::m_instance = nullptr; @@ -664,26 +672,40 @@ QString ScPaths::scrapbookDir(bool createIfNotExists) QString ScPaths::tempFileDir() { -#ifdef Q_OS_WIN32 - QString tempPath; - WCHAR wTempPath[1024]; - DWORD result = GetTempPathW(1024, wTempPath); - if (result) + return tempDir.path() + '/'; +} + +char ScPaths::doLockFile() +{ + // Lock as long as this Scribus runs. + lockFile.setStaleLockTime(0); + + // The values are never used. + // This is just to lock the file as early as possible + // so no other Scribus cleans up this Scribuses tempDir + return ScPaths::lockFile.tryLock(500) ? '1' : '0'; +}; + +void ScPaths::cleanupTemp() +{ + QDir tempDirOneUp(tempDir.path()); + tempDirOneUp.cdUp(); + QStringList searchFor(tempDirPrefix + "*"); + QStringList scribusTempDirs(tempDirOneUp.entryList(searchFor)); + for (QString tmpDirName : scribusTempDirs) { - tempPath = QString::fromUtf16((const unsigned short*) wTempPath); - tempPath.replace('\\', '/'); - tempPath += "/"; - // GetTempPath may return Windows directory, better not use this one - // for temporary files - if (QDir(tempPath).exists() && tempPath != windowsSpecialDir(CSIDL_WINDOWS)) - return tempPath; + QLockFile lockTester(tempDirOneUp.absolutePath() + "/" + tmpDirName + "/" + lockFilename); + lockTester.setStaleLockTime(500); + if (lockTester.tryLock()) + { + // The lock is aquired. + // This means the Scribus that once used the temporary + // directory containing the lockfile is no longer running. + lockTester.unlock(); + QDir deleteThis(tempDirOneUp.absolutePath() + "/" + tmpDirName); + deleteThis.removeRecursively(); + } } -#endif - - QDir tempAppDirectory(applicationDataDir() + "temp/"); - if (!tempAppDirectory.exists()) - tempAppDirectory.mkpath(tempAppDirectory.absolutePath()); - return tempAppDirectory.absolutePath() + "/"; } QString ScPaths::downloadDir() diff --git a/scribus/scribusapp.cpp b/scribus/scribusapp.cpp index db7ae2903..d61b67d29 100644 --- a/scribus/scribusapp.cpp +++ b/scribus/scribusapp.cpp @@ -107,6 +107,7 @@ ScribusQApp::~ScribusQApp() { delete m_ScCore; delete m_scDLMgr; + ScPaths::cleanupTemp(); LanguageManager::deleteInstance(); } |
|
i'm not skilled enough in the topic, to tell if the patch is correct, but from a bird's-eye-view it looks ok. i'll let @mrb or @jghali review it for correctness... |
Date Modified | Username | Field | Change |
---|---|---|---|
2024-11-22 14:16 | tropion | New Issue | |
2024-11-22 14:16 | tropion | File Added: tempDir.diff | |
2025-03-23 20:18 | ale | Note Added: 0052307 | |
2025-04-30 11:59 | tropion | Note Added: 0052485 | |
2025-04-30 16:24 | ale | Note Added: 0052486 | |
2025-05-20 14:59 | tropion | Note Added: 0052625 | |
2025-05-22 06:34 | ale | Note Added: 0052640 | |
2025-06-03 13:44 | tropion | Note Added: 0052696 | |
2025-06-03 13:44 | tropion | File Added: tempDir-withCleanup.diff | |
2025-06-03 16:27 | ale | Note Added: 0052698 | |
2025-06-05 10:16 | tropion | Note Added: 0052701 | |
2025-06-05 10:16 | tropion | File Added: tempDir-3.diff | |
2025-06-05 12:25 | ale | Note Added: 0052702 | |
2025-06-07 07:26 | ale | Summary | Scribuses working in parallel overwrite each others temp files => [PATCH] Scribuses working in parallel overwrite each others temp files |