diff --git a/src/app/crash/backup_observer.cpp b/src/app/crash/backup_observer.cpp index c7e2036b2..4334fc2bd 100644 --- a/src/app/crash/backup_observer.cpp +++ b/src/app/crash/backup_observer.cpp @@ -104,8 +104,12 @@ void BackupObserver::backgroundThread() for (app::Document* doc : m_documents) { try { - if (doc->needsBackup()) - m_session->saveDocumentChanges(doc); + if (doc->needsBackup()) { + 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()); diff --git a/src/app/crash/session.cpp b/src/app/crash/session.cpp index 6925c0c58..974848e13 100644 --- a/src/app/crash/session.cpp +++ b/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(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) diff --git a/src/app/crash/session.h b/src/app/crash/session.h index ac9b9b667..c9f2ef4ad 100644 --- a/src/app/crash/session.h +++ b/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); diff --git a/src/app/crash/write_document.cpp b/src/app/crash/write_document.cpp index dae1700de..dbf866c40 100644 --- a/src/app/crash/write_document.cpp +++ b/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 g_docVersions; +static std::map > 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); - saveObject("celdata", cel->data(), &Writer::writeCelData); + if (!saveObject("img", cel->image(), &Writer::writeImage)) + 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 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); - saveObject("doc", m_doc, &Writer::writeDocumentFile); + if (!saveObject("spr", spr, &Writer::writeSprite)) + 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) { - write32(s, doc->sprite()->id()); - write_string(s, doc->filename()); + bool isCanceled() const { + return (m_cancel && m_cancel->isCanceled()); } - 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(lay->flags())); // Flags write16(s, static_cast(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) { - write_image(s, img); + bool writeImage(std::ofstream& s, Image* 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 - 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)) - base::delete_file(oldfn); - } - catch (const std::exception&) { - TRACE(" - Cannot delete %s #%d v%d\n", prefix, obj->id(), versions.older()); - } + if (versions.older() && base::is_file(oldfn)) + m_deleteFiles.push_back(oldfn); // 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& 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.saveDocument(); + Writer writer(dir, doc, cancel); + 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 diff --git a/src/app/crash/write_document.h b/src/app/crash/write_document.h index 87c7fe651..d4a211992 100644 --- a/src/app/crash/write_document.h +++ b/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 +namespace doc { + class CancelIO; +} + namespace app { -class Document; -namespace crash { + class Document; - void write_document(const std::string& dir, app::Document* doc); - void delete_document_internals(app::Document* doc); + namespace crash { -} // 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 diff --git a/src/app/document_access.h b/src/app/document_access.h index 1e4f68cc6..abc041d2b 100644 --- a/src/app/document_access.h +++ b/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 diff --git a/src/app/rw_lock.cpp b/src/app/rw_lock.cpp index c7864db71..667f35606 100644 --- a/src/app/rw_lock.cpp +++ b/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 diff --git a/src/app/rw_lock.h b/src/app/rw_lock.h index 7497bd8e7..bfb5eca9d 100644 --- a/src/app/rw_lock.h +++ b/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); }; diff --git a/src/app/rw_lock_tests.cpp b/src/app/rw_lock_tests.cpp index 3e21f8363..7a593bb71 100644 --- a/src/app/rw_lock_tests.cpp +++ b/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(); +} diff --git a/src/app/ui/editor/editor.cpp b/src/app/ui/editor/editor.cpp index 2216b9ce6..be9c83bbe 100644 --- a/src/app/ui/editor/editor.cpp +++ b/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. - DocumentReader documentReader(m_document, 0); + // Lock the sprite to read/render it. We wait 1/4 secs in case + // 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())); diff --git a/src/app/ui/timeline.cpp b/src/app/ui/timeline.cpp index 04ce57e61..1979a46dc 100644 --- a/src/app/ui/timeline.cpp +++ b/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. - const DocumentReader documentReader(m_document, 0); + // Lock the sprite to read/render it. We wait 1/4 secs in case + // 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; diff --git a/src/doc/cancel_io.h b/src/doc/cancel_io.h new file mode 100644 index 000000000..f26277295 --- /dev/null +++ b/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 diff --git a/src/doc/image_io.cpp b/src/doc/image_io.cpp index 264e17023..32fafe342 100644 --- a/src/doc/image_io.cpp +++ b/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; yheight(); 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) diff --git a/src/doc/image_io.h b/src/doc/image_io.h index a1abef434..8145bdf0b 100644 --- a/src/doc/image_io.h +++ b/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