From 1bed018ab08b0c87da8b81b040802d3785d87adf Mon Sep 17 00:00:00 2001 From: David Capello Date: Fri, 10 Apr 2015 11:10:42 -0300 Subject: [PATCH] Add timeout parameter to Context/Document lock/unlock operations --- src/app/commands/cmd_import_sprite_sheet.cpp | 2 +- src/app/context_access.h | 20 ++--- src/app/context_flags.cpp | 4 +- src/app/crash/session.cpp | 4 +- src/app/document.cpp | 84 ++++++++++++-------- src/app/document.h | 4 +- src/app/document_access.h | 57 ++++--------- src/app/ui/document_view.cpp | 2 +- src/app/ui/editor/editor.cpp | 2 +- src/app/ui/editor/tool_loop_impl.cpp | 41 ++++------ src/app/ui/status_bar.cpp | 2 +- src/app/ui/timeline.cpp | 2 +- 12 files changed, 99 insertions(+), 125 deletions(-) diff --git a/src/app/commands/cmd_import_sprite_sheet.cpp b/src/app/commands/cmd_import_sprite_sheet.cpp index 92fbae537..62227480c 100644 --- a/src/app/commands/cmd_import_sprite_sheet.cpp +++ b/src/app/commands/cmd_import_sprite_sheet.cpp @@ -169,7 +169,7 @@ private: releaseEditor(); if (m_fileOpened) { - DocumentDestroyer destroyer(m_context, oldDocument); + DocumentDestroyer destroyer(m_context, oldDocument, 100); destroyer.destroyDocument(); } } diff --git a/src/app/context_access.h b/src/app/context_access.h index b05ac6808..78ad8847c 100644 --- a/src/app/context_access.h +++ b/src/app/context_access.h @@ -45,17 +45,17 @@ namespace app { } protected: - ContextAccess(const Context* context) + ContextAccess(const Context* context, int timeout) : m_context(context) - , m_document(context->activeDocument()) + , m_document(context->activeDocument(), timeout) , m_location(context->activeLocation()) { } template - ContextAccess(const Context* context, const DocumentReaderT& documentReader) + ContextAccess(const Context* context, const DocumentReaderT& documentReader, int timeout) : m_context(context) - , m_document(documentReader) + , m_document(documentReader, timeout) , m_location(context->activeLocation()) { } @@ -70,8 +70,8 @@ namespace app { // active document. class ContextReader : public ContextAccess { public: - ContextReader(const Context* context) - : ContextAccess(context) { + ContextReader(const Context* context, int timeout = 0) + : ContextAccess(context, timeout) { } }; @@ -79,12 +79,12 @@ namespace app { // active document. class ContextWriter : public ContextAccess { public: - ContextWriter(const Context* context) - : ContextAccess(context) { + ContextWriter(const Context* context, int timeout = 0) + : ContextAccess(context, timeout) { } - ContextWriter(const ContextReader& reader) - : ContextAccess(reader.context(), reader.document()) { + ContextWriter(const ContextReader& reader, int timeout = 0) + : ContextAccess(reader.context(), reader.document(), timeout) { } }; diff --git a/src/app/context_flags.cpp b/src/app/context_flags.cpp index 3ea2d92a0..bf3f01e8c 100644 --- a/src/app/context_flags.cpp +++ b/src/app/context_flags.cpp @@ -35,7 +35,7 @@ void ContextFlags::update(Context* context) if (document) { m_flags |= HasActiveDocument; - if (document->lock(Document::ReadLock)) { + if (document->lock(Document::ReadLock, 0)) { m_flags |= ActiveDocumentIsReadable; if (document->isMaskVisible()) @@ -75,7 +75,7 @@ void ContextFlags::update(Context* context) } } - if (document->lockToWrite()) + if (document->lockToWrite(0)) m_flags |= ActiveDocumentIsWritable; document->unlock(); diff --git a/src/app/crash/session.cpp b/src/app/crash/session.cpp index c4dc98100..7e883fd68 100644 --- a/src/app/crash/session.cpp +++ b/src/app/crash/session.cpp @@ -134,8 +134,8 @@ void Session::removeFromDisk() void Session::saveDocumentChanges(app::Document* doc) { - DocumentReader reader(doc); - DocumentWriter writer(reader); + DocumentReader reader(doc, 250); + DocumentWriter writer(reader, 250); app::Context ctx; std::string dir = base::join_path(m_path, base::convert_to(doc->id())); diff --git a/src/app/document.cpp b/src/app/document.cpp index 084ceb4be..a4e06b82c 100644 --- a/src/app/document.cpp +++ b/src/app/document.cpp @@ -23,6 +23,7 @@ #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" @@ -510,31 +511,42 @@ Document* Document::duplicate(DuplicateType type) const ////////////////////////////////////////////////////////////////////// // Multi-threading ("sprite wrappers" use this) -bool Document::lock(LockType lockType) +bool Document::lock(LockType lockType, int timeout) { - scoped_lock lock(*m_mutex); + while (timeout >= 0) { + { + scoped_lock lock(*m_mutex); + switch (lockType) { - 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; + TRACE("Document::lock: Locked <%d> to write\n", id()); + return true; + } + break; - 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; - TRACE("Document::lock: Locked <%d> to write\n", id()); - return true; - } + if (timeout > 0) { + int delay = MIN(100, timeout); + timeout -= delay; + base::this_thread::sleep_for(double(delay) / 1000.0); + } + else break; - } TRACE("Document::lock: Cannot lock <%d> to %s (has %d read locks and %d write locks)\n", @@ -542,23 +554,27 @@ bool Document::lock(LockType lockType) return false; } -bool Document::lockToWrite() +bool Document::lockToWrite(int timeout) { - scoped_lock lock(*m_mutex); + 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; + TRACE("Document::lockToWrite: Locked <%d> to write\n", id()); + return true; + } + } + timeout -= 100; + base::this_thread::sleep_for(0.100); + } - // 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; - TRACE("Document::lockToWrite: Locked <%d> to write\n", id()); - return true; - } - else { - TRACE("Document::lockToWrite: Cannot lock <%d> to write (has %d read locks and %d write locks)\n", - id(), m_read_locks, m_write_lock); - return false; - } + TRACE("Document::lockToWrite: Cannot lock <%d> to write (has %d read locks and %d write locks)\n", + id(), m_read_locks, m_write_lock); + return false; } void Document::unlockToRead() diff --git a/src/app/document.h b/src/app/document.h index aa5383203..828c478c3 100644 --- a/src/app/document.h +++ b/src/app/document.h @@ -170,11 +170,11 @@ namespace app { // 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); + 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(); + bool lockToWrite(int timeout); // If you've locked the sprite to write, using this method you can // your access level to only read it. diff --git a/src/app/document_access.h b/src/app/document_access.h index f4a83bc23..51a39fa22 100644 --- a/src/app/document_access.h +++ b/src/app/document_access.h @@ -69,35 +69,20 @@ namespace app { { } - explicit DocumentReader(Document* document) + explicit DocumentReader(Document* document, int timeout) : DocumentAccess(document) { - if (m_document && !m_document->lock(Document::ReadLock)) + if (m_document && !m_document->lock(Document::ReadLock, timeout)) throw LockedDocumentException(); } - explicit DocumentReader(const DocumentReader& copy) + explicit DocumentReader(const DocumentReader& copy, int timeout) : DocumentAccess(copy) { - if (m_document && !m_document->lock(Document::ReadLock)) + if (m_document && !m_document->lock(Document::ReadLock, timeout)) throw LockedDocumentException(); } - DocumentReader& operator=(const DocumentReader& copy) - { - // unlock old document - if (m_document) - m_document->unlock(); - - DocumentAccess::operator=(copy); - - // relock the document - if (m_document && !m_document->lock(Document::ReadLock)) - throw LockedDocumentException(); - - return *this; - } - ~DocumentReader() { // unlock the document @@ -105,6 +90,9 @@ namespace app { m_document->unlock(); } + private: + // Disable operator= + DocumentReader& operator=(const DocumentReader&); }; // Class to modify the document's state. Its constructor request a @@ -120,13 +108,13 @@ namespace app { { } - explicit DocumentWriter(Document* document) + explicit DocumentWriter(Document* document, int timeout) : DocumentAccess(document) , m_from_reader(false) , m_locked(false) { if (m_document) { - if (!m_document->lock(Document::WriteLock)) + if (!m_document->lock(Document::WriteLock, timeout)) throw LockedDocumentException(); m_locked = true; @@ -135,13 +123,13 @@ namespace app { // Constructor that can be used to elevate the given reader-lock to // writer permission. - explicit DocumentWriter(const DocumentReader& document) + explicit DocumentWriter(const DocumentReader& document, int timeout) : DocumentAccess(document) , m_from_reader(true) , m_locked(false) { if (m_document) { - if (!m_document->lockToWrite()) + if (!m_document->lockToWrite(timeout)) throw LockedDocumentException(); m_locked = true; @@ -153,24 +141,6 @@ namespace app { unlockWriter(); } - DocumentWriter& operator=(const DocumentReader& copy) - { - unlockWriter(); - - DocumentAccess::operator=(copy); - - if (m_document) { - m_from_reader = true; - - if (!m_document->lockToWrite()) - throw LockedDocumentException(); - - m_locked = true; - } - - return *this; - } - protected: void unlockWriter() @@ -191,13 +161,14 @@ namespace app { // Non-copyable DocumentWriter(const DocumentWriter&); DocumentWriter& operator=(const DocumentWriter&); + DocumentWriter& operator=(const DocumentReader&); }; // Used to destroy the active document in the context. class DocumentDestroyer : public DocumentWriter { public: - explicit DocumentDestroyer(Context* context, Document* document) - : DocumentWriter(document) + explicit DocumentDestroyer(Context* context, Document* document, int timeout) + : DocumentWriter(document, timeout) { } diff --git a/src/app/ui/document_view.cpp b/src/app/ui/document_view.cpp index db99d40ca..d2302d535 100644 --- a/src/app/ui/document_view.cpp +++ b/src/app/ui/document_view.cpp @@ -290,7 +290,7 @@ bool DocumentView::onCloseView(Workspace* workspace) // Destroy the sprite (locking it as writer) DocumentDestroyer destroyer( - static_cast(m_document->context()), m_document); + static_cast(m_document->context()), m_document, 250); StatusBar::instance() ->setStatusText(0, "Sprite '%s' closed.", diff --git a/src/app/ui/editor/editor.cpp b/src/app/ui/editor/editor.cpp index ba32cb0a8..216565bd6 100644 --- a/src/app/ui/editor/editor.cpp +++ b/src/app/ui/editor/editor.cpp @@ -1318,7 +1318,7 @@ void Editor::onPaint(ui::PaintEvent& ev) else { try { // Lock the sprite to read/render it. - DocumentReader documentReader(m_document); + DocumentReader documentReader(m_document, 250); // Draw the sprite in the editor drawSpriteUnclippedRect(g, gfx::Rect(0, 0, m_sprite->width(), m_sprite->height())); diff --git a/src/app/ui/editor/tool_loop_impl.cpp b/src/app/ui/editor/tool_loop_impl.cpp index 804b20b7b..c68f86770 100644 --- a/src/app/ui/editor/tool_loop_impl.cpp +++ b/src/app/ui/editor/tool_loop_impl.cpp @@ -191,20 +191,13 @@ public: if (!m_canceled) { // Paint ink if (getInk()->isPaint()) { - for (int i=0; ; ++i) { - // TODO add a "wait_n_seconds" parameter to ContextReader/Writer - try { - ContextReader reader(m_context); - ContextWriter writer(reader); - m_expandCelCanvas.commit(); - break; - } - catch (const LockedDocumentException& ex) { - if (i == 10) - Console::showException(ex); - else - base::this_thread::sleep_for(0.10); - } + try { + ContextReader reader(m_context, 500); + ContextWriter writer(reader, 500); + m_expandCelCanvas.commit(); + } + catch (const LockedDocumentException& ex) { + Console::showException(ex); } } // Selection ink @@ -220,19 +213,13 @@ public: // If the trace was canceled or it is not a 'paint' ink... if (m_canceled || !getInk()->isPaint()) { - for (int i=0; ; ++i) { - try { - ContextReader reader(m_context); - ContextWriter writer(reader); - m_expandCelCanvas.rollback(); - break; - } - catch (const LockedDocumentException& ex) { - if (i == 10) - Console::showException(ex); - else - base::this_thread::sleep_for(0.10); - } + try { + ContextReader reader(m_context, 500); + ContextWriter writer(reader, 500); + m_expandCelCanvas.rollback(); + } + catch (const LockedDocumentException& ex) { + Console::showException(ex); } } diff --git a/src/app/ui/status_bar.cpp b/src/app/ui/status_bar.cpp index cbdbbeb3f..cf6607bf6 100644 --- a/src/app/ui/status_bar.cpp +++ b/src/app/ui/status_bar.cpp @@ -450,7 +450,7 @@ void StatusBar::updateFromDocument(Editor* editor) { try { if (editor && editor->document()) { - const DocumentReader reader(editor->document()); + const DocumentReader reader(editor->document(), 100); m_commandsBox->setVisible(true); // Cel opacity diff --git a/src/app/ui/timeline.cpp b/src/app/ui/timeline.cpp index 3771d2517..71b9a08d6 100644 --- a/src/app/ui/timeline.cpp +++ b/src/app/ui/timeline.cpp @@ -809,7 +809,7 @@ void Timeline::onPaint(ui::PaintEvent& ev) try { // Lock the sprite to read/render it. - const DocumentReader documentReader(m_document); + const DocumentReader documentReader(m_document, 250); LayerIndex layer, first_layer, last_layer; frame_t frame, first_frame, last_frame;