View Issue Details

IDProjectCategoryView StatusLast Update
0017319ScribusImport / Exportpublic2025-06-07 07:26
Reportertropion Assigned To 
PrioritynormalSeverityminorReproducibilitysometimes
Status newResolutionopen 
Product Version1.7.0.svn 
Summary0017319: [PATCH] Scribuses working in parallel overwrite each others temp files
DescriptionI 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()
TagsNo tags attached.
PatchYes

Activities

tropion

2024-11-22 14:16

reporter  

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();
 }
 
tempDir.diff (1,334 bytes)   

ale

2025-03-23 20:18

manager   ~0052307

i guess that we would need some code to clean up the old directory that have been left around by scribuses that have crashed...

tropion

2025-04-30 11:59

reporter   ~0052485

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().

ale

2025-04-30 16:24

manager   ~0052486

I wonder if Scribus shouldn't use https://doc.qt.io/qt-6/qtemporarydir.html ...

Or are there good reasons for avoiding it?

tropion

2025-05-20 14:59

reporter   ~0052625

I see no reason to avoid QTemporaryDir. I just didn't realise that we had such nice things available.

ale

2025-05-22 06:34

manager   ~0052640

would you mind integrating the newly acquired knowledge into your patch and check if it fits your needs?

tropion

2025-06-03 13:44

reporter   ~0052696

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();
 }
 
tempDir-withCleanup.diff (5,375 bytes)   

ale

2025-06-03 16:27

manager   ~0052698

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" ....

tropion

2025-06-05 10:16

reporter   ~0052701

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();
 }
 
tempDir-3.diff (4,542 bytes)   

ale

2025-06-05 12:25

manager   ~0052702

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...

Issue History

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