A Tx now will always try to lock the document if possible (#2430)

With re-entrant RWLocks we can try to lock the document on each
transaction/command/modification. This fixes several problems running
scripts that weren't locking the sprite in an app.transaction() call.

laf

@@ -1 +1 @@
-Subproject commit b8edff525c083c250ba7558c0df83d91e34e0f0e
+Subproject commit 157533d1600d424e089be859148aa71a169111f2

src/app/context_flags.cpp

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2019-2022  Igara Studio S.A.
+// Copyright (C) 2019-2023  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -36,7 +36,8 @@ void ContextFlags::update(Context* context)
   if (document) {
     m_flags |= HasActiveDocument;
 
-    if (document->readLock(0)) {
+    Doc::LockResult res = document->readLock(0);
+    if (res != Doc::LockResult::Fail) {
       m_flags |= ActiveDocumentIsReadable;
 
       if (document->isMaskVisible())
@@ -47,7 +48,7 @@ void ContextFlags::update(Context* context)
       if (document->canWriteLockFromRead() && !document->isReadOnly())
         m_flags |= ActiveDocumentIsWritable;
 
-      document->unlock();
+      document->unlock(res);
     }
 
 #ifdef ENABLE_UI

src/app/doc.cpp

@@ -41,7 +41,7 @@
 #include <limits>
 #include <map>
 
-#define DOC_TRACE(...) // TRACEARGS
+#define DOC_TRACE(...) // TRACEARGS(__VA_ARGS__)
 
 namespace app {
 
@@ -102,38 +102,50 @@ bool Doc::canWriteLockFromRead() const
   return m_rwLock.canWriteLockFromRead();
 }
 
-bool Doc::readLock(int timeout)
+Doc::LockResult Doc::readLock(int timeout)
 {
-  return m_rwLock.lock(base::RWLock::ReadLock, timeout);
+  auto res = m_rwLock.lock(base::RWLock::ReadLock, timeout);
+  DOC_TRACE("DOC: readLock", this, (int)res);
+  return res;
 }
 
-bool Doc::writeLock(int timeout)
+Doc::LockResult Doc::writeLock(int timeout)
 {
-  return m_rwLock.lock(base::RWLock::WriteLock, timeout);
+  auto res = m_rwLock.lock(base::RWLock::WriteLock, timeout);
+  DOC_TRACE("DOC: writeLock", this, (int)res);
+  return res;
 }
 
-bool Doc::upgradeToWrite(int timeout)
+Doc::LockResult Doc::upgradeToWrite(int timeout)
 {
-  return m_rwLock.upgradeToWrite(timeout);
+  auto res = m_rwLock.upgradeToWrite(timeout);
+  DOC_TRACE("DOC: upgradeToWrite", this, (int)res);
+  return res;
 }
 
-void Doc::downgradeToRead()
+void Doc::downgradeToRead(LockResult lockResult)
 {
-  m_rwLock.downgradeToRead();
+  DOC_TRACE("DOC: downgradeToRead", this, (int)lockResult);
+  m_rwLock.downgradeToRead(lockResult);
 }
 
-void Doc::unlock()
+void Doc::unlock(LockResult lockResult)
 {
-  m_rwLock.unlock();
+  ASSERT(lockResult != base::RWLock::LockResult::Fail);
+  DOC_TRACE("DOC: unlock", this, (int)lockResult);
+  m_rwLock.unlock(lockResult);
 }
 
 bool Doc::weakLock(std::atomic<base::RWLock::WeakLock>* weak_lock_flag)
 {
-  return m_rwLock.weakLock(weak_lock_flag);
+  bool res = m_rwLock.weakLock(weak_lock_flag);
+  DOC_TRACE("DOC: weakLock", this, (int)res);
+  return res;
 }
 
 void Doc::weakUnlock()
 {
+  DOC_TRACE("DOC: weakUnlock", this);
   m_rwLock.weakUnlock();
 }
 

src/app/doc.h

@@ -68,6 +68,8 @@ namespace app {
       kReadOnly         = 16,// This document is read-only
     };
   public:
+    using LockResult = base::RWLock::LockResult;
+
     Doc(Sprite* sprite);
     ~Doc();
 
@@ -76,11 +78,11 @@ namespace app {
 
     // Lock/unlock API (RWLock wrapper)
     bool canWriteLockFromRead() const;
-    bool readLock(int timeout);
-    bool writeLock(int timeout);
-    bool upgradeToWrite(int timeout);
-    void downgradeToRead();
-    void unlock();
+    LockResult readLock(int timeout);
+    LockResult writeLock(int timeout);
+    LockResult upgradeToWrite(int timeout);
+    void downgradeToRead(LockResult lockResult);
+    void unlock(LockResult lockResult);
 
     bool weakLock(std::atomic<base::RWLock::WeakLock>* weak_lock_flag);
     void weakUnlock();

src/app/doc_access.h

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2019-2020  Igara Studio S.A.
+// Copyright (C) 2019-2023  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -46,6 +46,8 @@ namespace app {
   // locks.
   class DocAccess {
   public:
+    using LockResult = Doc::LockResult;
+
     DocAccess() : m_doc(NULL) { }
     DocAccess(const DocAccess& copy) : m_doc(copy.m_doc) { }
     explicit DocAccess(Doc* doc) : m_doc(doc) { }
@@ -71,6 +73,7 @@ namespace app {
 
   protected:
     Doc* m_doc;
+    LockResult m_lockResult = LockResult::Fail;
   };
 
   // Class to view the document's state. Its constructor request a
@@ -83,14 +86,20 @@ namespace app {
 
     explicit DocReader(Doc* doc, int timeout)
       : DocAccess(doc) {
-      if (m_doc && !m_doc->readLock(timeout))
-        throw CannotReadDocException();
+      if (m_doc) {
+        m_lockResult = m_doc->readLock(timeout);
+        if (m_lockResult == LockResult::Fail)
+          throw CannotReadDocException();
+      }
     }
 
     explicit DocReader(const DocReader& copy, int timeout)
       : DocAccess(copy) {
-      if (m_doc && !m_doc->readLock(timeout))
-        throw CannotReadDocException();
+      if (m_doc) {
+        m_lockResult = m_doc->readLock(timeout);
+        if (m_lockResult == LockResult::Fail)
+          throw CannotReadDocException();
+      }
     }
 
     ~DocReader() {
@@ -100,7 +109,7 @@ namespace app {
   protected:
     void unlock() {
       if (m_doc) {
-        m_doc->unlock();
+        m_doc->unlock(m_lockResult);
         m_doc = nullptr;
       }
     }
@@ -127,7 +136,8 @@ namespace app {
       , m_from_reader(false)
       , m_locked(false) {
       if (m_doc) {
-        if (!m_doc->writeLock(timeout))
+        m_lockResult = m_doc->writeLock(timeout);
+        if (m_lockResult == LockResult::Fail)
           throw CannotWriteDocException();
 
         m_locked = true;
@@ -141,7 +151,8 @@ namespace app {
       , m_from_reader(true)
       , m_locked(false) {
       if (m_doc) {
-        if (!m_doc->upgradeToWrite(timeout))
+        m_lockResult = m_doc->upgradeToWrite(timeout);
+        if (m_lockResult == LockResult::Fail)
           throw CannotWriteDocException();
 
         m_locked = true;
@@ -156,9 +167,9 @@ namespace app {
     void unlock() {
       if (m_doc && m_locked) {
         if (m_from_reader)
-          m_doc->downgradeToRead();
+          m_doc->downgradeToRead(m_lockResult);
         else
-          m_doc->unlock();
+          m_doc->unlock(m_lockResult);
 
         m_doc = nullptr;
         m_locked = false;

src/app/script/app_object.cpp

@@ -157,25 +157,23 @@ int App_transaction(lua_State* L)
     if (!ctx)
       return luaL_error(L, "no context");
 
-    // Create a new transaction so it exists in the whole duration of
-    // the argument function call.
-#if 0
-    // TODO To be able to lock the document in the transaction we have
-    // to create a re-entrant RWLock implementation to allow to call
-    // commands (creating sub-ContextWriters) inside the
-    // app.transaction()
-    ContextWriter writer(ctx);
-    Tx tx(writer, label);
-#else
-    Tx tx(Tx::DontLockDoc, ctx, ctx->activeDocument(), label);
-#endif
+    try {
+      // We lock the document in the whole transaction because the
+      // RWLock now is re-entrant and we are able to call commands
+      // inside the app.transaction() (creating inner ContextWriters).
+      ContextWriter writer(ctx);
+      Tx tx(writer, label);
 
-    lua_pushvalue(L, -1);
-    if (lua_pcall(L, 0, LUA_MULTRET, 0) == LUA_OK)
-      tx.commit();
-    else
-      return lua_error(L); // pcall already put an error object on the stack
-    nresults = lua_gettop(L) - top;
+      lua_pushvalue(L, -1);
+      if (lua_pcall(L, 0, LUA_MULTRET, 0) == LUA_OK)
+        tx.commit();
+      else
+        return lua_error(L); // pcall already put an error object on the stack
+      nresults = lua_gettop(L) - top;
+    }
+    catch (const LockedDocException& ex) {
+      return luaL_error(L, "cannot lock document for transaction\n%s", ex.what());
+    }
   }
   return nresults;
 }

src/app/tx.h

@@ -46,17 +46,19 @@ namespace app {
       if (!m_doc)
         throw std::runtime_error("No document to execute a transaction");
 
+      // Lock the document here (even if we are inside a transaction,
+      // this will be a re-entrant lock if we can)
+      if (lockAction == LockDoc) {
+        m_lockResult = m_doc->writeLock(500);
+        if (m_lockResult == Doc::LockResult::Fail)
+          throw CannotWriteDocException();
+      }
+
       m_transaction = m_doc->transaction();
       if (m_transaction) {
         m_owner = false;
       }
       else {
-        if (lockAction == LockDoc) {
-          if (!m_doc->writeLock(500))
-            throw CannotWriteDocException();
-          m_locked = true;
-        }
-
         m_transaction = new Transaction(ctx, m_doc, label, mod);
         m_doc->setTransaction(m_transaction);
         m_owner = true;
@@ -92,10 +94,10 @@ namespace app {
       if (m_owner) {
         m_doc->setTransaction(nullptr);
         delete m_transaction;
-
-        if (m_locked)
-          m_doc->unlock();
       }
+
+      if (m_lockResult != Doc::LockResult::Fail)
+        m_doc->unlock(m_lockResult);
     }
 
     void commit() {
@@ -127,7 +129,7 @@ namespace app {
   private:
     Doc* m_doc;
     Transaction* m_transaction;
-    bool m_locked = false; // The doc was locked here
+    Doc::LockResult m_lockResult = Doc::LockResult::Fail;
     bool m_owner = false;  // Owner of the transaction
   };