Bug 756241 - Race condition in parallel DOT runs

3 files changed, 77 insertions, 55 deletions

diff --git a/qtools/Doxyfile b/qtools/Doxyfile
index 41f2ad9..2943270 100644
--- a/qtools/Doxyfile
+++ b/qtools/Doxyfile
@@ -279,7 +279,7 @@ PERL_PATH              = /usr/bin/perl
 CLASS_DIAGRAMS         = YES
 MSCGEN_PATH            =
 HIDE_UNDOC_RELATIONS   = YES
-HAVE_DOT               = NO
+HAVE_DOT               = YES
 DOT_NUM_THREADS        = 0
 DOT_FONTNAME           =
 DOT_FONTSIZE           = 10
@@ -306,4 +306,4 @@ MAX_DOT_GRAPH_DEPTH    = 0
 DOT_TRANSPARENT        = YES
 DOT_MULTI_TARGETS      = NO
 GENERATE_LEGEND        = YES
-DOT_CLEANUP            = NO
+DOT_CLEANUP            = YES
diff --git a/src/dot.cpp b/src/dot.cpp
index 701f868..2f245f6 100644
--- a/src/dot.cpp
+++ b/src/dot.cpp
@@ -659,10 +659,9 @@ static bool writeSVGFigureLink(FTextStream &out,const QCString &relPath,
 
 // since dot silently reproduces the input file when it does not
 // support the PNG format, we need to check the result.
-static void checkDotResult(const QCString &imgName)
+static void checkDotResult(const char *imgExt, const char *imgName)
 {
-  QCString imgExt = getDotImageExtension();
-  if (imgExt=="png")
+  if (qstrcmp(imgExt,"png")==0)
   {
     FILE *f = portable_fopen(imgName,"rb");
     if (f)
@@ -675,19 +674,19 @@ static void checkDotResult(const QCString &imgName)
           err("Image `%s' produced by dot is not a valid PNG!\n"
               "You should either select a different format "
               "(DOT_IMAGE_FORMAT in the config file) or install a more "
-              "recent version of graphviz (1.7+)\n",imgName.data()
+              "recent version of graphviz (1.7+)\n",imgName
              );
         }
       }
       else
       {
-        err("Could not read image `%s' generated by dot!\n",imgName.data());
+        err("Could not read image `%s' generated by dot!\n",imgName);
       }
       fclose(f);
     }
     else
     {
-      err("Could not open image `%s' generated by dot!\n",imgName.data());
+      err("Could not open image `%s' generated by dot!\n",imgName);
     }
   }
 }
@@ -793,54 +792,46 @@ int DotNodeList::compareValues(const DotNode *n1,const DotNode *n2) const
 
 DotRunner::DotRunner(const QCString &file,const QCString &path,
                      bool checkResult,const QCString &imageName) 
-  : m_file(file), m_path(path), 
-    m_checkResult(checkResult), m_imageName(imageName)
-{
-  static bool dotCleanUp = Config_getBool("DOT_CLEANUP"); 
-  m_cleanUp = dotCleanUp;
+  : m_dotExe(Config_getString("DOT_PATH")+"dot"),
+    m_file(file), m_path(path), 
+    m_checkResult(checkResult), m_imageName(imageName),
+    m_imgExt(getDotImageExtension())
+{
+  static bool dotCleanUp      = Config_getBool("DOT_CLEANUP"); 
+  static bool dotMultiTargets = Config_getBool("DOT_MULTI_TARGETS");
+  m_cleanUp      = dotCleanUp;
+  m_multiTargets = dotMultiTargets;
   m_jobs.setAutoDelete(TRUE);
 }
 
 void DotRunner::addJob(const char *format,const char *output)
 {
   QCString args = QCString("-T")+format+" -o \""+output+"\"";
-  m_jobs.append(new QCString(args));
+  m_jobs.append(new DotConstString(args));
 }
 
 void DotRunner::addPostProcessing(const char *cmd,const char *args)
 {
-  m_postCmd = cmd;
-  m_postArgs = args;
+  m_postCmd.set(cmd);
+  m_postArgs.set(args);
 }
 
 bool DotRunner::run()
 {
   int exitCode=0;
-  // we need to use data here to make a copy of the string, as Config_getString can be called by
-  // multiple threads simulaneously and the reference counting is not thread safe.
-  QCString dotExe   = Config_getString("DOT_PATH").data();
-  dotExe+="dot";
 
-  bool multiTargets = Config_getBool("DOT_MULTI_TARGETS");
   QCString dotArgs;
-  QListIterator<QCString> li(m_jobs);
-  QCString *s;
-  QCString file      = m_file;
-  QCString path      = m_path;
-  QCString imageName = m_imageName;
-  QCString postCmd   = m_postCmd;
-  QCString postArgs  = m_postArgs;
-  bool checkResult   = m_checkResult;
-  bool cleanUp       = m_cleanUp;
-  if (multiTargets)
-  {
-    dotArgs="\""+file+"\"";
+  QListIterator<DotConstString> li(m_jobs);
+  DotConstString *s;
+  if (m_multiTargets)
+  {
+    dotArgs=QCString("\"")+m_file.data()+"\"";
     for (li.toFirst();(s=li.current());++li)
     {
       dotArgs+=' ';
-      dotArgs+=*s;
+      dotArgs+=s->data();
     }
-    if ((exitCode=portable_system(dotExe,dotArgs,FALSE))!=0)
+    if ((exitCode=portable_system(m_dotExe.data(),dotArgs,FALSE))!=0)
     {
       goto error;
     }
@@ -849,30 +840,33 @@ bool DotRunner::run()
   {
     for (li.toFirst();(s=li.current());++li)
     {
-      dotArgs="\""+file+"\" "+*s;
-      if ((exitCode=portable_system(dotExe,dotArgs,FALSE))!=0)
+      dotArgs=QCString("\"")+m_file.data()+"\" "+s->data();
+      if ((exitCode=portable_system(m_dotExe.data(),dotArgs,FALSE))!=0)
       {
         goto error;
       }
     }
   }
-  if (!postCmd.isEmpty() && portable_system(postCmd,postArgs)!=0)
+  if (!m_postCmd.isEmpty() && portable_system(m_postCmd.data(),m_postArgs.data())!=0)
   {
     err("Problems running '%s' as a post-processing step for dot output\n",m_postCmd.data());
     return FALSE;
   }
-  if (checkResult) checkDotResult(imageName);
-  if (cleanUp) 
+  if (m_checkResult)
+  {
+    checkDotResult(m_imgExt.data(),m_imageName.data());
+  }
+  if (m_cleanUp) 
   {
     //printf("removing dot file %s\n",m_file.data());
     //QDir(path).remove(file);
-    m_cleanupItem.file = file;
-    m_cleanupItem.path = path;
+    m_cleanupItem.file.set(m_file.data());
+    m_cleanupItem.path.set(m_path.data());
   }
   return TRUE;
 error:
   err("Problems running dot: exit code=%d, command='%s', arguments='%s'\n",
-      exitCode,dotExe.data(),dotArgs.data());
+      exitCode,m_dotExe.data(),dotArgs.data());
   return FALSE;
 }
 
@@ -1202,7 +1196,7 @@ void DotWorkerThread::run()
   while ((runner=m_queue->dequeue()))
   {
     runner->run();
-    DotRunner::CleanupItem cleanup = runner->cleanup();
+    const DotRunner::CleanupItem &cleanup = runner->cleanup();
     if (!cleanup.file.isEmpty())
     {
       m_cleanupItems.append(new DotRunner::CleanupItem(cleanup));
@@ -1216,7 +1210,7 @@ void DotWorkerThread::cleanup()
   DotRunner::CleanupItem *ci;
   for (;(ci=it.current());++it)
   {
-    QDir(ci->path).remove(ci->file);
+    QDir(ci->path.data()).remove(ci->file.data());
   }
 }
 
@@ -4237,7 +4231,7 @@ void writeDotGraphFromFile(const char *inFile,const char *outDir,
      return;
   }
 
-  if (format==GOF_BITMAP) checkDotResult(absImgName);
+  if (format==GOF_BITMAP) checkDotResult(getDotImageExtension(),absImgName);
 
   Doxygen::indexList->addImageFile(imgName);
 
diff --git a/src/dot.h b/src/dot.h
index 64d6dd4..e12d547 100644
--- a/src/dot.h
+++ b/src/dot.h
@@ -326,6 +326,31 @@ class DotGroupCollaboration
     QList<Edge>     m_edges;
 };
 
+/** Minimal constant string class that is thread safe, once initialized. */
+class DotConstString
+{
+  public:
+    DotConstString()                                   { m_str=0; }
+   ~DotConstString()                                   { delete[] m_str; }
+    DotConstString(const QCString &s) : m_str(0)       { set(s); }
+    DotConstString(const DotConstString &s) : m_str(0) { set(s.data()); }
+    const char *data() const                           { return m_str; }
+    bool isEmpty() const                               { return m_str==0 || m_str[0]=='\0'; }
+    void set(const QCString &s)
+    {
+      delete[] m_str;
+      m_str=0;
+      if (!s.isEmpty())
+      {
+        m_str=new char[s.length()+1];
+        qstrcpy(m_str,s.data());
+      }
+    }
+  private:
+    DotConstString &operator=(const DotConstString &);
+    char *m_str;
+};
+
 /** Helper class to run dot from doxygen.
  */
 class DotRunner
@@ -333,8 +358,8 @@ class DotRunner
   public:
     struct CleanupItem
     {
-      QCString path;
-      QCString file;
+      DotConstString path;
+      DotConstString file;
     };
 
     /** Creates a runner for a dot \a file. */
@@ -352,16 +377,19 @@ class DotRunner
 
     /** Runs dot for all jobs added. */
     bool run();
-    CleanupItem cleanup() const { return m_cleanupItem; }
+    const CleanupItem &cleanup() const { return m_cleanupItem; }
 
   private:
-    QList<QCString> m_jobs;
-    QCString m_postArgs;
-    QCString m_postCmd;
-    QCString m_file;
-    QCString m_path;
+    DotConstString m_dotExe;
+    bool m_multiTargets;
+    QList<DotConstString> m_jobs;
+    DotConstString m_postArgs;
+    DotConstString m_postCmd;
+    DotConstString m_file;
+    DotConstString m_path;
     bool m_checkResult;
-    QCString m_imageName;
+    DotConstString m_imageName;
+    DotConstString m_imgExt;
     bool m_cleanUp;
     CleanupItem m_cleanupItem;
 };