Move doc lock/unlock logic to a new RWLock class

src/app/CMakeLists.txt

@@ -324,6 +324,7 @@ add_library(app-lib
   res/palettes_loader_delegate.cpp
   res/resources_loader.cpp
   resource_finder.cpp
+  rw_lock.cpp
   script/app_object.cpp
   script/app_scripting.cpp
   script/console_object.cpp

src/app/context_flags.cpp

@@ -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.
@@ -44,7 +44,7 @@ void ContextFlags::update(Context* context)
 
       updateFlagsFromSite(site);
 
-      if (document->lockToWrite(0))
+      if (document->upgradeToWrite(0))
         m_flags |= ActiveDocumentIsWritable;
 
       document->unlock();

src/app/document.cpp

@@ -8,10 +8,6 @@
 #include "config.h"
 #endif
 
-// Uncomment this line in case that you want TRACE() lock/unlock
-// operations.
-//#define DEBUG_DOCUMENT_LOCKS
-
 #include "app/document.h"
 
 #include "app/app.h"
@@ -25,9 +21,6 @@
 #include "app/pref/preferences.h"
 #include "app/util/create_cel_copy.h"
 #include "base/memory.h"
-#include "base/mutex.h"
-#include "base/scoped_lock.h"
-#include "base/thread.h"
 #include "base/unique_ptr.h"
 #include "doc/cel.h"
 #include "doc/context.h"
@@ -50,8 +43,6 @@ using namespace doc;
 Document::Document(Sprite* sprite)
   : m_undo(new DocumentUndo)
   , m_associated_to_file(false)
-  , m_write_lock(false)
-  , m_read_locks(0)
     // Information about the file format used to load/save this document
   , m_format_options(NULL)
   // Mask
@@ -439,130 +430,6 @@ Document* Document::duplicate(DuplicateType type) const
   return documentCopy.release();
 }
 
-//////////////////////////////////////////////////////////////////////
-// Multi-threading ("sprite wrappers" use this)
-
-bool Document::lock(LockType lockType, int timeout)
-{
-  while (timeout >= 0) {
-    {
-      scoped_lock lock(m_mutex);
-      switch (lockType) {
-
-        case ReadLock:
-          // If no body is writting the sprite...
-          if (!m_write_lock) {
-            // We can read it
-            ++m_read_locks;
-            return true;
-          }
-          break;
-
-        case WriteLock:
-          // If no body is reading and writting...
-          if (m_read_locks == 0 && !m_write_lock) {
-            // We can start writting the sprite...
-            m_write_lock = true;
-
-#ifdef DEBUG_DOCUMENT_LOCKS
-            TRACE("DOC: Document::lock: Locked <%d> to write\n", id());
-#endif
-            return true;
-          }
-          break;
-
-      }
-    }
-
-    if (timeout > 0) {
-      int delay = MIN(100, timeout);
-      timeout -= delay;
-
-#ifdef DEBUG_DOCUMENT_LOCKS
-      TRACE("DOC: Document::lock: wait 100 msecs for <%d>\n", id());
-#endif
-
-      base::this_thread::sleep_for(double(delay) / 1000.0);
-    }
-    else
-      break;
-  }
-
-#ifdef DEBUG_DOCUMENT_LOCKS
-  TRACE("DOC: Document::lock: Cannot lock <%d> to %s (has %d read locks and %d write locks)\n",
-    id(), (lockType == ReadLock ? "read": "write"), m_read_locks, m_write_lock);
-#endif
-
-  return false;
-}
-
-bool Document::lockToWrite(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_DOCUMENT_LOCKS
-        TRACE("DOC: Document::lockToWrite: Locked <%d> to write\n", id());
-#endif
-
-        return true;
-      }
-    }
-
-    if (timeout > 0) {
-      int delay = MIN(100, timeout);
-      timeout -= delay;
-
-#ifdef DEBUG_DOCUMENT_LOCKS
-      TRACE("DOC: Document::lockToWrite: wait 100 msecs for <%d>\n", id());
-#endif
-
-      base::this_thread::sleep_for(double(delay) / 1000.0);
-    }
-    else
-      break;
-  }
-
-#ifdef DEBUG_DOCUMENT_LOCKS
-  TRACE("DOC: Document::lockToWrite: Cannot lock <%d> to write (has %d read locks and %d write locks)\n",
-    id(), m_read_locks, m_write_lock);
-#endif
-
-  return false;
-}
-
-void Document::unlockToRead()
-{
-  scoped_lock lock(m_mutex);
-
-  ASSERT(m_read_locks == 0);
-  ASSERT(m_write_lock);
-
-  m_write_lock = false;
-  m_read_locks = 1;
-}
-
-void Document::unlock()
-{
-  scoped_lock lock(m_mutex);
-
-  if (m_write_lock) {
-    m_write_lock = false;
-  }
-  else if (m_read_locks > 0) {
-    --m_read_locks;
-  }
-  else {
-    ASSERT(false);
-  }
-}
-
 void Document::onContextChanged()
 {
   m_undo->setContext(context());

src/app/document.h

@@ -10,6 +10,7 @@
 
 #include "app/extra_cel.h"
 #include "app/file/format_options.h"
+#include "app/rw_lock.h"
 #include "app/transformation.h"
 #include "base/disable_copying.h"
 #include "base/mutex.h"
@@ -50,13 +51,9 @@ namespace app {
 
   // An application document. It is the class used to contain one file
   // opened and being edited by the user (a sprite).
-  class Document : public doc::Document {
+  class Document : public doc::Document,
+                   public RWLock {
   public:
-    enum LockType {
-      ReadLock,
-      WriteLock
-    };
-
     Document(Sprite* sprite);
     ~Document();
 
@@ -157,23 +154,6 @@ namespace app {
     void copyLayerContent(const Layer* sourceLayer, Document* destDoc, Layer* destLayer) const;
     Document* duplicate(DuplicateType type) const;
 
-    //////////////////////////////////////////////////////////////////////
-    // Multi-threading ("sprite wrappers" use this)
-
-    // Locks the sprite to read or write on it, returning true if the
-    // sprite can be accessed in the desired mode.
-    bool lock(LockType lockType, int timeout);
-
-    // If you've locked the sprite to read, using this method you can
-    // raise your access level to write it.
-    bool lockToWrite(int timeout);
-
-    // If you've locked the sprite to write, using this method you can
-    // your access level to only read it.
-    void unlockToRead();
-
-    void unlock();
-
   protected:
     virtual void onContextChanged() override;
 
@@ -187,15 +167,6 @@ namespace app {
     // Selected mask region boundaries
     base::UniquePtr<doc::MaskBoundaries> m_maskBoundaries;
 
-    // Mutex to modify the 'locked' flag.
-    base::mutex m_mutex;
-
-    // True if some thread is writing the sprite.
-    bool m_write_lock;
-
-    // Greater than zero when one or more threads are reading the sprite.
-    int m_read_locks;
-
     // Data to save the file in the same format that it was loaded
     base::SharedPtr<FormatOptions> m_format_options;
 

src/app/document_access.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.
@@ -16,6 +16,7 @@
 
 namespace app {
 
+  // TODO remove exceptions and use "DocumentAccess::operator bool()"
   class LockedDocumentException : public base::Exception {
   public:
     LockedDocumentException(const char* msg) throw()
@@ -141,7 +142,7 @@ namespace app {
       , m_from_reader(true)
       , m_locked(false) {
       if (m_document) {
-        if (!m_document->lockToWrite(timeout))
+        if (!m_document->upgradeToWrite(timeout))
           throw CannotWriteDocumentException();
 
         m_locked = true;
@@ -156,7 +157,7 @@ namespace app {
     void unlock() {
       if (m_document && m_locked) {
         if (m_from_reader)
-          m_document->unlockToRead();
+          m_document->downgradeToRead();
         else
           m_document->unlock();
 

src/app/rw_lock.cpp

@@ -0,0 +1,157 @@
+// Aseprite
+// Copyright (C) 2001-2016  David Capello
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+// Uncomment this line in case that you want TRACE() lock/unlock
+// operations.
+//#define DEBUG_OBJECT_LOCKS
+
+#include "app/document.h"
+
+#include "base/scoped_lock.h"
+#include "base/thread.h"
+
+namespace app {
+
+using namespace base;
+
+RWLock::RWLock()
+  : m_write_lock(false)
+  , m_read_locks(0)
+{
+}
+
+RWLock::~RWLock()
+{
+  ASSERT(!m_write_lock);
+  ASSERT(m_read_locks == 0);
+}
+
+bool RWLock::lock(LockType lockType, int timeout)
+{
+  while (timeout >= 0) {
+    {
+      scoped_lock lock(m_mutex);
+      switch (lockType) {
+
+        case ReadLock:
+          // If no body is writting the object...
+          if (!m_write_lock) {
+            // We can read it
+            ++m_read_locks;
+            return true;
+          }
+          break;
+
+        case WriteLock:
+          // If no body is reading and writting...
+          if (m_read_locks == 0 && !m_write_lock) {
+            // We can start writting the object...
+            m_write_lock = true;
+
+#ifdef DEBUG_OBJECT_LOCKS
+            TRACE("LCK: lock: Locked <%p> to write\n", this);
+#endif
+            return true;
+          }
+          break;
+
+      }
+    }
+
+    if (timeout > 0) {
+      int delay = MIN(100, timeout);
+      timeout -= delay;
+
+#ifdef DEBUG_OBJECT_LOCKS
+      TRACE("LCK: lock: 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: lock: Cannot lock <%p> to %s (has %d read locks and %d write locks)\n",
+    this, (lockType == ReadLock ? "read": "write"), m_read_locks, m_write_lock);
+#endif
+
+  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);
+
+  ASSERT(m_read_locks == 0);
+  ASSERT(m_write_lock);
+
+  m_write_lock = false;
+  m_read_locks = 1;
+}
+
+void RWLock::unlock()
+{
+  scoped_lock lock(m_mutex);
+
+  if (m_write_lock) {
+    m_write_lock = false;
+  }
+  else if (m_read_locks > 0) {
+    --m_read_locks;
+  }
+  else {
+    ASSERT(false);
+  }
+}
+
+} // namespace app

src/app/rw_lock.h

@@ -0,0 +1,57 @@
+// Aseprite
+// Copyright (C) 2001-2016  David Capello
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#ifndef APP_RW_LOCK_H_INCLUDED
+#define APP_RW_LOCK_H_INCLUDED
+#pragma once
+
+#include "base/disable_copying.h"
+#include "base/mutex.h"
+
+namespace app {
+
+  // A readers-writer lock implementation
+  class RWLock {
+  public:
+    enum LockType {
+      ReadLock,
+      WriteLock
+    };
+
+    RWLock();
+    ~RWLock();
+
+    // Locks the object to read or write on it, returning true if the
+    // object can be accessed in the desired mode.
+    bool lock(LockType lockType, int timeout);
+
+    // If you've locked the object to read, using this method you can
+    // raise your access level to write it.
+    bool upgradeToWrite(int timeout);
+
+    // If we've locked the object to write, using this method we can
+    // lower our access to read-only.
+    void downgradeToRead();
+
+    // Unlocks a previously successfully lock() operation.
+    void unlock();
+
+  private:
+    // Mutex to modify the 'locked' flag.
+    base::mutex m_mutex;
+
+    // True if some thread is writing the object.
+    bool m_write_lock;
+
+    // Greater than zero when one or more threads are reading the object.
+    int m_read_locks;
+
+    DISABLE_COPYING(RWLock);
+  };
+
+} // namespace app
+
+#endif

src/app/rw_lock_tests.cpp

@@ -0,0 +1,61 @@
+// Aseprite
+// Copyright (C) 2001-2016  David Capello
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#include "tests/test.h"
+
+#include "app/rw_lock.h"
+
+using namespace app;
+
+TEST(RWLock, MultipleReaders)
+{
+  RWLock a;
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.unlock();
+  a.unlock();
+  a.unlock();
+  a.unlock();
+}
+
+TEST(RWLock, OneWriter)
+{
+  RWLock a;
+
+  EXPECT_TRUE(a.lock(RWLock::WriteLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::ReadLock, 0));
+  a.unlock();
+
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.unlock();
+
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.unlock();
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.unlock();
+  EXPECT_TRUE(a.lock(RWLock::WriteLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.unlock();
+}
+
+TEST(RWLock, UpgradeToWrite)
+{
+  RWLock a;
+
+  EXPECT_TRUE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  EXPECT_TRUE(a.upgradeToWrite(0));
+  EXPECT_FALSE(a.lock(RWLock::ReadLock, 0));
+  EXPECT_FALSE(a.lock(RWLock::WriteLock, 0));
+  a.downgradeToRead();
+  a.unlock();
+}