Unlock document from backup thread when UI needs it

Added a new "weak lock" concept to RWLock/Document class. The background
thread that saves data recovery information can obtain this "weak lock"
which can be unlocked by the UI thread in any moment (i.e. when the user
needs the document immediately for UI.)

src/app/crash/backup_observer.cpp

@@ -104,8 +104,12 @@ void BackupObserver::backgroundThread()
 
       for (app::Document* doc : m_documents) {
         try {
-          if (doc->needsBackup())
+          if (doc->needsBackup()) {
-            m_session->saveDocumentChanges(doc);
+            if (!m_session->saveDocumentChanges(doc)) {
+              TRACE("RECO: Document '%d' backup was canceled by UI\n", doc->id());
+              somethingLocked = true;
+            }
+          }
         }
         catch (const std::exception&) {
           TRACE("RECO: Document '%d' is locked\n", doc->id());

src/app/crash/session.cpp

@@ -26,6 +26,7 @@
 #include "base/split_string.h"
 #include "base/string.h"
 #include "base/unique_ptr.h"
+#include "doc/cancel_io.h"
 
 namespace app {
 namespace crash {
@@ -128,9 +129,25 @@ void Session::removeFromDisk()
   }
 }
 
-void Session::saveDocumentChanges(app::Document* doc)
+class CustomWeakDocumentReader : public WeakDocumentReader
+                               , public doc::CancelIO {
+public:
+  explicit CustomWeakDocumentReader(Document* doc)
+    : WeakDocumentReader(doc) {
+  }
+
+  // CancelIO impl
+  bool isCanceled() override {
+    return !isLocked();
+  }
+};
+
+bool Session::saveDocumentChanges(app::Document* doc)
 {
-  DocumentReader reader(doc, 250);
+  CustomWeakDocumentReader reader(doc);
+  if (!reader.isLocked())
+    return false;
+
   app::Context ctx;
   std::string dir = base::join_path(m_path,
     base::convert_to<std::string>(doc->id()));
@@ -140,7 +157,7 @@ void Session::saveDocumentChanges(app::Document* doc)
     base::make_directory(dir);
 
   // Save document information
-  write_document(dir, doc);
+  return write_document(dir, doc, &reader);
 }
 
 void Session::removeDocument(app::Document* doc)

src/app/crash/session.h

@@ -49,7 +49,7 @@ namespace crash {
     void create(base::pid pid);
     void removeFromDisk();
 
-    void saveDocumentChanges(app::Document* doc);
+    bool saveDocumentChanges(app::Document* doc);
     void removeDocument(app::Document* doc);
 
     void restoreBackup(Backup* backup);

src/app/crash/write_document.cpp

@@ -18,6 +18,7 @@
 #include "base/serialization.h"
 #include "base/string.h"
 #include "base/unique_ptr.h"
+#include "doc/cancel_io.h"
 #include "doc/cel.h"
 #include "doc/cel_data_io.h"
 #include "doc/cel_io.h"
@@ -45,52 +46,74 @@ using namespace doc;
 namespace {
 
 static std::map<ObjectId, ObjVersionsMap> g_docVersions;
+static std::map<ObjectId, std::vector<std::string> > g_deleteFiles;
 
 class Writer {
 public:
-  Writer(const std::string& dir, app::Document* doc)
+  Writer(const std::string& dir, app::Document* doc, doc::CancelIO* cancel)
     : m_dir(dir)
     , m_doc(doc)
-    , m_objVersions(g_docVersions[doc->id()]) {
+    , m_objVersions(g_docVersions[doc->id()])
+    , m_deleteFiles(g_deleteFiles[doc->id()])
+    , m_cancel(cancel) {
   }
 
-  void saveDocument() {
+  bool saveDocument() {
     Sprite* spr = m_doc->sprite();
 
     // Save from objects without children (e.g. images), to aggregated
     // objects (e.g. cels, layers, etc.)
 
     for (Palette* pal : spr->getPalettes())
-      saveObject("pal", pal, &Writer::writePalette);
+      if (!saveObject("pal", pal, &Writer::writePalette))
+        return false;
 
     for (FrameTag* frtag : spr->frameTags())
-      saveObject("frtag", frtag, &Writer::writeFrameTag);
+      if (!saveObject("frtag", frtag, &Writer::writeFrameTag))
+        return false;
 
     for (Cel* cel : spr->uniqueCels()) {
-      saveObject("img", cel->image(), &Writer::writeImage);
+      if (!saveObject("img", cel->image(), &Writer::writeImage))
-      saveObject("celdata", cel->data(), &Writer::writeCelData);
+        return false;
+
+      if (!saveObject("celdata", cel->data(), &Writer::writeCelData))
+        return false;
     }
 
     for (Cel* cel : spr->cels())
-      saveObject("cel", cel, &Writer::writeCel);
+      if (!saveObject("cel", cel, &Writer::writeCel))
+        return false;
 
     std::vector<Layer*> layers;
     spr->getLayersList(layers);
     for (Layer* lay : layers)
-      saveObject("lay", lay, &Writer::writeLayerStructure);
+      if (!saveObject("lay", lay, &Writer::writeLayerStructure))
+        return false;
 
-    saveObject("spr", spr, &Writer::writeSprite);
+    if (!saveObject("spr", spr, &Writer::writeSprite))
-    saveObject("doc", m_doc, &Writer::writeDocumentFile);
+      return false;
+
+    if (!saveObject("doc", m_doc, &Writer::writeDocumentFile))
+      return false;
+
+    // Delete old files after all files are correctly saved.
+    deleteOldVersions();
+    return true;
   }
 
 private:
 
-  void writeDocumentFile(std::ofstream& s, app::Document* doc) {
+  bool isCanceled() const {
-    write32(s, doc->sprite()->id());
+    return (m_cancel && m_cancel->isCanceled());
-    write_string(s, doc->filename());
   }
 
-  void writeSprite(std::ofstream& s, Sprite* spr) {
+  bool writeDocumentFile(std::ofstream& s, app::Document* doc) {
+    write32(s, doc->sprite()->id());
+    write_string(s, doc->filename());
+    return true;
+  }
+
+  bool writeSprite(std::ofstream& s, Sprite* spr) {
     write8(s, spr->pixelFormat());
     write16(s, spr->width());
     write16(s, spr->height());
@@ -117,9 +140,11 @@ private:
     write32(s, spr->frameTags().size());
     for (FrameTag* frtag : spr->frameTags())
       write32(s, frtag->id());
+
+    return true;
   }
 
-  void writeLayerStructure(std::ofstream& s, Layer* lay) {
+  bool writeLayerStructure(std::ofstream& s, Layer* lay) {
     write32(s, static_cast<int>(lay->flags())); // Flags
     write16(s, static_cast<int>(lay->type()));  // Type
     write_string(s, lay->name());
@@ -135,36 +160,44 @@ private:
         write32(s, cel->id());
       }
     }
+    return true;
   }
 
-  void writeCel(std::ofstream& s, Cel* cel) {
+  bool writeCel(std::ofstream& s, Cel* cel) {
     write_cel(s, cel);
+    return true;
   }
 
-  void writeCelData(std::ofstream& s, CelData* celdata) {
+  bool writeCelData(std::ofstream& s, CelData* celdata) {
     write_celdata(s, celdata);
+    return true;
   }
 
-  void writeImage(std::ofstream& s, Image* img) {
+  bool writeImage(std::ofstream& s, Image* img) {
-    write_image(s, img);
+    return write_image(s, img, m_cancel);
   }
 
-  void writePalette(std::ofstream& s, Palette* pal) {
+  bool writePalette(std::ofstream& s, Palette* pal) {
     write_palette(s, pal);
+    return true;
   }
 
-  void writeFrameTag(std::ofstream& s, FrameTag* frameTag) {
+  bool writeFrameTag(std::ofstream& s, FrameTag* frameTag) {
     write_frame_tag(s, frameTag);
+    return true;
   }
 
   template<typename T>
-  void saveObject(const char* prefix, T* obj, void (Writer::*writeMember)(std::ofstream&, T*)) {
+  bool saveObject(const char* prefix, T* obj, bool (Writer::*writeMember)(std::ofstream&, T*)) {
+    if (isCanceled())
+      return false;
+
     if (!obj->version())
       obj->incrementVersion();
 
     ObjVersions& versions = m_objVersions[obj->id()];
     if (versions.newer() == obj->version())
-      return;
+      return true;
 
     std::string fn = prefix;
     fn.push_back('-');
@@ -176,7 +209,8 @@ private:
 
     std::ofstream s(FSTREAM_PATH(fullfn), std::ofstream::binary);
     write32(s, 0);                // Leave a room for the magic number
-    (this->*writeMember)(s, obj); // Write the object
+    if (!(this->*writeMember)(s, obj)) // Write the object
+      return false;
 
     // Flush all data. In this way we ensure that the magic number is
     // the last thing being written in the file.
@@ -187,23 +221,36 @@ private:
     write32(s, MAGIC_NUMBER);
 
     // Remove the older version
-    try {
+    if (versions.older() && base::is_file(oldfn))
-      if (versions.older() && base::is_file(oldfn))
+      m_deleteFiles.push_back(oldfn);
-        base::delete_file(oldfn);
-    }
-    catch (const std::exception&) {
-      TRACE(" - Cannot delete %s #%d v%d\n", prefix, obj->id(), versions.older());
-    }
 
     // Rotate versions and add the latest one
     versions.rotateRevisions(obj->version());
 
     TRACE(" - Saved %s #%d v%d\n", prefix, obj->id(), obj->version());
+    return true;
+  }
+
+  void deleteOldVersions() {
+    while (!m_deleteFiles.empty() && !isCanceled()) {
+      std::string file = m_deleteFiles.back();
+      m_deleteFiles.erase(m_deleteFiles.end()-1);
+
+      try {
+        TRACE(" - Deleting <%s>\n", file.c_str());
+        base::delete_file(file);
+      }
+      catch (const std::exception&) {
+        TRACE(" - Cannot delete <%s>\n", file.c_str());
+      }
+    }
   }
 
   std::string m_dir;
   app::Document* m_doc;
   ObjVersionsMap& m_objVersions;
+  std::vector<std::string>& m_deleteFiles;
+  doc::CancelIO* m_cancel;
 };
 
 } // anonymous namespace
@@ -211,21 +258,30 @@ private:
 //////////////////////////////////////////////////////////////////////
 // Public API
 
-void write_document(const std::string& dir, app::Document* doc)
+bool write_document(const std::string& dir,
+                    app::Document* doc,
+                    doc::CancelIO* cancel)
 {
-  Writer writer(dir, doc);
+  Writer writer(dir, doc, cancel);
-  writer.saveDocument();
+  return writer.saveDocument();
 }
 
 void delete_document_internals(app::Document* doc)
 {
   ASSERT(doc);
-  auto it = g_docVersions.find(doc->id());
 
   // The document could not be inside g_documentObjects in case it was
   // never saved by the backup process.
-  if (it != g_docVersions.end())
+  {
-    g_docVersions.erase(it);
+    auto it = g_docVersions.find(doc->id());
+    if (it != g_docVersions.end())
+      g_docVersions.erase(it);
+  }
+  {
+    auto it = g_deleteFiles.find(doc->id());
+    if (it != g_deleteFiles.end())
+      g_deleteFiles.erase(it);
+  }
 }
 
 } // namespace crash

src/app/crash/write_document.h

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2001-2015  David Capello
+// Copyright (C) 2001-2016  David Capello
 //
 // This program is distributed under the terms of
 // the End-User License Agreement for Aseprite.
@@ -10,14 +10,19 @@
 
 #include <string>
 
+namespace doc {
+  class CancelIO;
+}
+
 namespace app {
-class Document;
+  class Document;
-namespace crash {
 
-  void write_document(const std::string& dir, app::Document* doc);
+  namespace crash {
-  void delete_document_internals(app::Document* doc);
 
-} // namespace crash
+    bool write_document(const std::string& dir, app::Document* doc, doc::CancelIO* cancel);
+    void delete_document_internals(app::Document* doc);
+
+  } // namespace crash
 } // namespace app
 
 #endif

src/app/document_access.h

@@ -196,6 +196,42 @@ namespace app {
 
   };
 
+  class WeakDocumentReader : public DocumentAccess {
+  public:
+    WeakDocumentReader() {
+    }
+
+    explicit WeakDocumentReader(Document* doc)
+      : DocumentAccess(doc)
+      , m_weak_lock(RWLock::WeakUnlocked) {
+      if (m_document)
+        m_document->weakLock(&m_weak_lock);
+    }
+
+    ~WeakDocumentReader() {
+      weakUnlock();
+    }
+
+    bool isLocked() const {
+      return (m_weak_lock == RWLock::WeakLocked);
+    }
+
+  protected:
+    void weakUnlock() {
+      if (m_document && m_weak_lock != RWLock::WeakUnlocked) {
+        m_document->weakUnlock();
+        m_document = nullptr;
+      }
+    }
+
+  private:
+    // Disable operator=
+    WeakDocumentReader(const WeakDocumentReader&);
+    WeakDocumentReader& operator=(const WeakDocumentReader&);
+
+    RWLock::WeakLock m_weak_lock;
+  };
+
 } // namespace app
 
 #endif

src/app/rw_lock.cpp

@@ -24,6 +24,7 @@ using namespace base;
 RWLock::RWLock()
   : m_write_lock(false)
   , m_read_locks(0)
+  , m_weak_lock(nullptr)
 {
 }
 
@@ -31,6 +32,7 @@ RWLock::~RWLock()
 {
   ASSERT(!m_write_lock);
   ASSERT(m_read_locks == 0);
+  ASSERT(m_weak_lock == nullptr);
 }
 
 bool RWLock::lock(LockType lockType, int timeout)
@@ -38,6 +40,19 @@ bool RWLock::lock(LockType lockType, int timeout)
   while (timeout >= 0) {
     {
       scoped_lock lock(m_mutex);
+
+      // Check that there is no weak lock
+      if (m_weak_lock) {
+        if (*m_weak_lock == WeakLocked)
+          *m_weak_lock = WeakUnlocking;
+
+        // Wait some time
+        if (*m_weak_lock == WeakUnlocking)
+          goto go_wait;
+
+        ASSERT(*m_weak_lock == WeakUnlocked);
+      }
+
       switch (lockType) {
 
         case ReadLock:
@@ -63,6 +78,8 @@ bool RWLock::lock(LockType lockType, int timeout)
           break;
 
       }
+
+    go_wait:;
     }
 
     if (timeout > 0) {
@@ -87,47 +104,6 @@ bool RWLock::lock(LockType lockType, int timeout)
   return false;
 }
 
-bool RWLock::upgradeToWrite(int timeout)
-{
-  while (timeout >= 0) {
-    {
-      scoped_lock lock(m_mutex);
-      // this only is possible if there are just one reader
-      if (m_read_locks == 1) {
-        ASSERT(!m_write_lock);
-        m_read_locks = 0;
-        m_write_lock = true;
-
-#ifdef DEBUG_OBJECT_LOCKS
-        TRACE("LCK: upgradeToWrite: Locked <%p> to write\n", this);
-#endif
-
-        return true;
-      }
-    }
-
-    if (timeout > 0) {
-      int delay = MIN(100, timeout);
-      timeout -= delay;
-
-#ifdef DEBUG_OBJECT_LOCKS
-      TRACE("LCK: upgradeToWrite: wait 100 msecs for <%p>\n", this);
-#endif
-
-      base::this_thread::sleep_for(double(delay) / 1000.0);
-    }
-    else
-      break;
-  }
-
-#ifdef DEBUG_OBJECT_LOCKS
-  TRACE("LCK: upgradeToWrite: Cannot lock <%p> to write (has %d read locks and %d write locks)\n",
-    this, m_read_locks, m_write_lock);
-#endif
-
-  return false;
-}
-
 void RWLock::downgradeToRead()
 {
   scoped_lock lock(m_mutex);
@@ -154,4 +130,87 @@ void RWLock::unlock()
   }
 }
 
+bool RWLock::weakLock(WeakLock* weak_lock_flag)
+{
+  scoped_lock lock(m_mutex);
+
+  if (m_weak_lock ||
+      m_write_lock ||
+      m_read_locks > 0)
+    return false;
+
+  m_weak_lock = weak_lock_flag;
+  *m_weak_lock = WeakLocked;
+  return true;
+}
+
+void RWLock::weakUnlock()
+{
+  ASSERT(m_weak_lock);
+  ASSERT(*m_weak_lock != WeakLock::WeakUnlocked);
+  ASSERT(!m_write_lock);
+  ASSERT(m_read_locks == 0);
+
+  if (m_weak_lock) {
+    *m_weak_lock = WeakLock::WeakUnlocked;
+    m_weak_lock = nullptr;
+  }
+}
+
+bool RWLock::upgradeToWrite(int timeout)
+{
+  while (timeout >= 0) {
+    {
+      scoped_lock lock(m_mutex);
+
+      // Check that there is no weak lock
+      if (m_weak_lock) {
+        if (*m_weak_lock == WeakLocked)
+          *m_weak_lock = WeakUnlocking;
+
+        // Wait some time
+        if (*m_weak_lock == WeakUnlocking)
+          goto go_wait;
+
+        ASSERT(*m_weak_lock == WeakUnlocked);
+      }
+
+      // this only is possible if there are just one reader
+      if (m_read_locks == 1) {
+        ASSERT(!m_write_lock);
+        m_read_locks = 0;
+        m_write_lock = true;
+
+#ifdef DEBUG_OBJECT_LOCKS
+        TRACE("LCK: upgradeToWrite: Locked <%p> to write\n", this);
+#endif
+
+        return true;
+      }
+
+    go_wait:;
+    }
+
+    if (timeout > 0) {
+      int delay = MIN(100, timeout);
+      timeout -= delay;
+
+#ifdef DEBUG_OBJECT_LOCKS
+      TRACE("LCK: upgradeToWrite: wait 100 msecs for <%p>\n", this);
+#endif
+
+      base::this_thread::sleep_for(double(delay) / 1000.0);
+    }
+    else
+      break;
+  }
+
+#ifdef DEBUG_OBJECT_LOCKS
+  TRACE("LCK: upgradeToWrite: Cannot lock <%p> to write (has %d read locks and %d write locks)\n",
+    this, m_read_locks, m_write_lock);
+#endif
+
+  return false;
+}
+
 } // namespace app

src/app/rw_lock.h

@@ -21,6 +21,12 @@ namespace app {
       WriteLock
     };
 
+    enum WeakLock {
+      WeakUnlocked,
+      WeakUnlocking,
+      WeakLocked,
+    };
+
     RWLock();
     ~RWLock();
 
@@ -39,6 +45,15 @@ namespace app {
     // Unlocks a previously successfully lock() operation.
     void unlock();
 
+    // Tries to lock the object for read access in a "weak way" so
+    // other thread (e.g. UI thread) can lock the object removing this
+    // weak lock.
+    //
+    // The "weak_lock_flag" is used to notify when the "weak lock" is
+    // lost.
+    bool weakLock(WeakLock* weak_lock_flag);
+    void weakUnlock();
+
   private:
     // Mutex to modify the 'locked' flag.
     base::mutex m_mutex;
@@ -49,6 +64,13 @@ namespace app {
     // Greater than zero when one or more threads are reading the object.
     int m_read_locks;
 
+    // If this isn' nullptr, it means that it points to an unique
+    // "weak" lock that can be unlocked from other thread. E.g. the
+    // backup/data recovery thread might weakly lock the object so if
+    // the user UI thread needs the object again, the backup process
+    // can stop.
+    WeakLock* m_weak_lock;
+
     DISABLE_COPYING(RWLock);
   };
 

src/app/rw_lock_tests.cpp

@@ -59,3 +59,20 @@ TEST(RWLock, UpgradeToWrite)
   a.downgradeToRead();
   a.unlock();
 }
+
+TEST(RWLock, WeakLock)
+{
+  RWLock a;
+  RWLock::WeakLock flag = RWLock::WeakUnlocked;
+
+  EXPECT_TRUE(a.weakLock(&flag));
+  EXPECT_EQ(RWLock::WeakLocked, flag);
+  EXPECT_FALSE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_EQ(RWLock::WeakUnlocking, flag);
+  a.weakUnlock();
+  EXPECT_EQ(RWLock::WeakUnlocked, flag);
+
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.weakLock(&flag));
+  a.unlock();
+}

src/app/ui/editor/editor.cpp

@@ -1400,8 +1400,9 @@ void Editor::onPaint(ui::PaintEvent& ev)
   // Editor with sprite
   else {
     try {
-      // Lock the sprite to read/render it.
+      // Lock the sprite to read/render it. We wait 1/4 secs in case
-      DocumentReader documentReader(m_document, 0);
+      // the background thread is making a backup.
+      DocumentReader documentReader(m_document, 250);
 
       // Draw the sprite in the editor
       drawSpriteUnclippedRect(g, gfx::Rect(0, 0, m_sprite->width(), m_sprite->height()));

src/app/ui/timeline.cpp

@@ -923,8 +923,9 @@ void Timeline::onPaint(ui::PaintEvent& ev)
     goto paintNoDoc;
 
   try {
-    // Lock the sprite to read/render it.
+    // Lock the sprite to read/render it. We wait 1/4 secs in case
-    const DocumentReader documentReader(m_document, 0);
+    // the background thread is making a backup.
+    const DocumentReader documentReader(m_document, 250);
 
     LayerIndex layer, first_layer, last_layer;
     frame_t frame, first_frame, last_frame;

src/doc/cancel_io.h

@@ -0,0 +1,21 @@
+// Aseprite Document Library
+// Copyright (c) 2001-2016 David Capello
+//
+// This file is released under the terms of the MIT license.
+// Read LICENSE.txt for more information.
+
+#ifndef DOC_CANCEL_IO_H_INCLUDED
+#define DOC_CANCEL_IO_H_INCLUDED
+#pragma once
+
+namespace doc {
+
+  class CancelIO {
+  public:
+    virtual ~CancelIO() { }
+    virtual bool isCanceled() = 0;
+  };
+
+} // namespace doc
+
+#endif

src/doc/image_io.cpp

@@ -14,6 +14,7 @@
 #include "base/exception.h"
 #include "base/serialization.h"
 #include "base/unique_ptr.h"
+#include "doc/cancel_io.h"
 #include "doc/image.h"
 #include "zlib.h"
 
@@ -26,7 +27,7 @@ using namespace base::serialization::little_endian;
 
 // TODO Create a zlib wrapper for iostreams
 
-void write_image(std::ostream& os, const Image* image)
+bool write_image(std::ostream& os, const Image* image, CancelIO* cancel)
 {
   write32(os, image->id());
   write8(os, image->pixelFormat());    // Pixel format
@@ -57,6 +58,11 @@ void write_image(std::ostream& os, const Image* image)
     int total_output_bytes = 0;
 
     for (int y=0; y<image->height(); y++) {
+      if (cancel && cancel->isCanceled()) {
+        deflateEnd(&zstream);
+        return false;
+      }
+
       zstream.next_in = (Bytef*)image->getPixelAddress(0, y);
       zstream.avail_in = rowSize;
       int flush = (y == image->height()-1 ? Z_FINISH: Z_NO_FLUSH);
@@ -90,6 +96,7 @@ void write_image(std::ostream& os, const Image* image)
     os.seekp(bak);
   }
 #endif
+  return true;
 }
 
 Image* read_image(std::istream& is, bool setId)

src/doc/image_io.h

@@ -1,5 +1,5 @@
 // Aseprite Document Library
-// Copyright (c) 2001-2015 David Capello
+// Copyright (c) 2001-2016 David Capello
 //
 // This file is released under the terms of the MIT license.
 // Read LICENSE.txt for more information.
@@ -12,9 +12,10 @@
 
 namespace doc {
 
+  class CancelIO;
   class Image;
 
-  void write_image(std::ostream& os, const Image* image);
+  bool write_image(std::ostream& os, const Image* image, CancelIO* cancel = nullptr);
   Image* read_image(std::istream& is, bool setId = true);
 
 } // namespace doc