From 57d7e3e2ab37ef3b5d2c1eef3509fc42fcfe0f2e Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 5 Mar 2021 13:07:52 +1000 Subject: [PATCH] commit immediately when there's no active checkpoint Reviews and operations on the backend that support undoing can now be committed immediately, so they will not be lost in the event of a crash. This required tweaks to a few places: - don't set collection mtime on save() unless changes were made in Python, as otherwise we end up accidentally clearing the backend undo queue - autosave() is now run on every reset() - garbage collection now runs in a timer, instead of relying on autosave() to be run periodically --- pylib/anki/collection.py | 59 ++++++++++++++++++++++++++-------------- pylib/anki/dbproxy.py | 6 ++-- pylib/tests/test_undo.py | 2 +- qt/aqt/about.py | 2 +- qt/aqt/addcards.py | 2 +- qt/aqt/browser.py | 4 +-- qt/aqt/clayout.py | 2 +- qt/aqt/editcurrent.py | 2 +- qt/aqt/main.py | 39 ++++++++++++++++++-------- qt/aqt/stats.py | 4 +-- 10 files changed, 79 insertions(+), 43 deletions(-) diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py index cd592be93..6ee215c08 100644 --- a/pylib/anki/collection.py +++ b/pylib/anki/collection.py @@ -90,9 +90,6 @@ class Collection: self.path = os.path.abspath(path) self.reopen() - self.log(self.path, anki.version) - self._lastSave = time.time() - self._undo: _UndoInfo = None self.media = MediaManager(self, server) self.models = ModelManager(self) self.decks = DeckManager(self) @@ -230,15 +227,18 @@ class Collection: # to check if the backend updated the modification time. return self.db.last_begin_at != self.mod - def save( - self, name: Optional[str] = None, mod: Optional[int] = None, trx: bool = True - ) -> None: + def save(self, name: Optional[str] = None, trx: bool = True) -> None: "Flush, commit DB, and take out another write lock if trx=True." # commit needed? - if self.db.mod or self.modified_after_begin(): - self.mod = intTime(1000) if mod is None else mod + if self.db.modified_in_python or self.modified_after_begin(): + if self.db.modified_in_python: + self.db.execute("update col set mod = ?", intTime(1000)) + self.db.modified_in_python = False + else: + # modifications made by the backend will have already bumped + # mtime + pass self.db.commit() - self.db.mod = False if trx: self.db.begin() elif not trx: @@ -247,14 +247,15 @@ class Collection: self.db.rollback() self._save_checkpoint(name) - self._lastSave = time.time() - def autosave(self) -> Optional[bool]: - "Save if 5 minutes has passed since last save. True if saved." - if time.time() - self._lastSave > 300: + def autosave(self) -> None: + """Save any pending changes. + If a checkpoint was taken in the last 5 minutes, don't save.""" + if not self._have_outstanding_checkpoint(): + # if there's no active checkpoint, we can save immediately + self.save() + elif time.time() - self._last_checkpoint_at > 300: self.save() - return True - return None def close(self, save: bool = True, downgrade: bool = False) -> None: "Disconnect from DB." @@ -290,6 +291,9 @@ class Collection: assert not self.db assert self.path.endswith(".anki2") + self._last_checkpoint_at = time.time() + self._undo: _UndoInfo = None + (media_dir, media_db) = media_paths_from_col_path(self.path) log_path = "" @@ -355,6 +359,7 @@ class Collection: def reset(self) -> None: "Rebuild the queue and reload data after DB modified." + self.autosave() self.sched.reset() # Deletion logging @@ -752,17 +757,16 @@ table.review-log {{ {revlog_style} }} def undo_status(self) -> UndoStatus: "Return the undo status. At the moment, redo is not supported." # check backend first - status = self._backend.get_undo_status() - if status.undo or status.redo: + if status := self._check_backend_undo_status(): return status if not self._undo: - return status + return UndoStatus() if isinstance(self._undo, _ReviewsUndo): - status.undo = self.tr(TR.SCHEDULING_REVIEW) + return UndoStatus(undo=self.tr(TR.SCHEDULING_REVIEW)) elif isinstance(self._undo, Checkpoint): - status.undo = self._undo.name + return UndoStatus(undo=self._undo.name) else: assert_exhaustive(self._undo) assert False @@ -796,6 +800,16 @@ table.review-log {{ {revlog_style} }} assert_exhaustive(self._undo) assert False + def _check_backend_undo_status(self) -> Optional[UndoStatus]: + """Return undo status if undo available on backend. + If backend has undo available, clear the Python undo state.""" + status = self._backend.get_undo_status() + if status.undo or status.redo: + self.clear_python_undo() + return status + else: + return None + def save_card_review_undo_info(self, card: Card) -> None: "Used by V1 and V2 schedulers to record state prior to review." if not isinstance(self._undo, _ReviewsUndo): @@ -805,6 +819,10 @@ table.review-log {{ {revlog_style} }} entry = ReviewUndo(card=copy.copy(card), was_leech=was_leech) self._undo.entries.append(entry) + def _have_outstanding_checkpoint(self) -> bool: + self._check_backend_undo_status() + return isinstance(self._undo, Checkpoint) + def _undo_checkpoint(self) -> Checkpoint: assert isinstance(self._undo, Checkpoint) self.rollback() @@ -814,6 +832,7 @@ table.review-log {{ {revlog_style} }} def _save_checkpoint(self, name: Optional[str]) -> None: "Call via .save(). If name not provided, clear any existing checkpoint." + self._last_checkpoint_at = time.time() if name: self._undo = Checkpoint(name=name) else: diff --git a/pylib/anki/dbproxy.py b/pylib/anki/dbproxy.py index 1bb4f9d95..3561613c5 100644 --- a/pylib/anki/dbproxy.py +++ b/pylib/anki/dbproxy.py @@ -24,7 +24,7 @@ class DBProxy: def __init__(self, backend: anki._backend.RustBackend) -> None: self._backend = backend - self.mod = False + self.modified_in_python = False self.last_begin_at = 0 # Transactions @@ -54,7 +54,7 @@ class DBProxy: s = sql.strip().lower() for stmt in "insert", "update", "delete": if s.startswith(stmt): - self.mod = True + self.modified_in_python = True sql, args2 = emulate_named_args(sql, args, kwargs) # fetch rows return self._backend.db_query(sql, args2, first_row_only) @@ -92,7 +92,7 @@ class DBProxy: ################ def executemany(self, sql: str, args: Iterable[Sequence[ValueForDB]]) -> None: - self.mod = True + self.modified_in_python = True if isinstance(args, list): list_args = args else: diff --git a/pylib/tests/test_undo.py b/pylib/tests/test_undo.py index fe323c67c..7bfaa2381 100644 --- a/pylib/tests/test_undo.py +++ b/pylib/tests/test_undo.py @@ -22,7 +22,7 @@ def test_op(): # it should be listed as undoable assert col.undoName() == "studyopts" # with about 5 minutes until it's clobbered - assert time.time() - col._lastSave < 1 + assert time.time() - col._last_checkpoint_at < 1 # undoing should restore the old value col.undo() assert not col.undoName() diff --git a/qt/aqt/about.py b/qt/aqt/about.py index d49a21a36..04d577c2d 100644 --- a/qt/aqt/about.py +++ b/qt/aqt/about.py @@ -28,7 +28,7 @@ class ClosableQDialog(QDialog): def show(mw: aqt.AnkiQt) -> QDialog: dialog = ClosableQDialog(mw) disable_help_button(dialog) - mw.setupDialogGC(dialog) + mw.garbage_collect_on_dialog_finish(dialog) abt = aqt.forms.about.Ui_About() abt.setupUi(dialog) diff --git a/qt/aqt/addcards.py b/qt/aqt/addcards.py index 47de845da..d5c44330d 100644 --- a/qt/aqt/addcards.py +++ b/qt/aqt/addcards.py @@ -34,7 +34,7 @@ from aqt.utils import ( class AddCards(QDialog): def __init__(self, mw: AnkiQt) -> None: QDialog.__init__(self, None, Qt.Window) - mw.setupDialogGC(self) + mw.garbage_collect_on_dialog_finish(self) self.mw = mw self.form = aqt.forms.addcards.Ui_Dialog() self.form.setupUi(self) diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index 9f1f40356..a0b3cb468 100644 --- a/qt/aqt/browser.py +++ b/qt/aqt/browser.py @@ -575,7 +575,7 @@ class Browser(QMainWindow): self.mw.maybeReset() aqt.dialogs.markClosed("Browser") self._closeEventHasCleanedUp = True - self.mw.gcWindow(self) + self.mw.deferred_delete_and_garbage_collect(self) self.close() def closeWithCallback(self, onsuccess: Callable) -> None: @@ -1556,7 +1556,7 @@ where id in %s""" def _onFindDupes(self) -> None: d = QDialog(self) - self.mw.setupDialogGC(d) + self.mw.garbage_collect_on_dialog_finish(d) frm = aqt.forms.finddupes.Ui_Dialog() frm.setupUi(d) restoreGeom(d, "findDupes") diff --git a/qt/aqt/clayout.py b/qt/aqt/clayout.py index 377fa2f47..01474cd21 100644 --- a/qt/aqt/clayout.py +++ b/qt/aqt/clayout.py @@ -47,7 +47,7 @@ class CardLayout(QDialog): fill_empty: bool = False, ) -> None: QDialog.__init__(self, parent or mw, Qt.Window) - mw.setupDialogGC(self) + mw.garbage_collect_on_dialog_finish(self) self.mw = aqt.mw self.note = note self.ord = ord diff --git a/qt/aqt/editcurrent.py b/qt/aqt/editcurrent.py index 2c3832663..4a540d76b 100644 --- a/qt/aqt/editcurrent.py +++ b/qt/aqt/editcurrent.py @@ -10,7 +10,7 @@ from aqt.utils import TR, disable_help_button, restoreGeom, saveGeom, tooltip, t class EditCurrent(QDialog): def __init__(self, mw: aqt.AnkiQt) -> None: QDialog.__init__(self, None, Qt.Window) - mw.setupDialogGC(self) + mw.garbage_collect_on_dialog_finish(self) self.mw = mw self.form = aqt.forms.editcurrent.Ui_Dialog() self.form.setupUi(self) diff --git a/qt/aqt/main.py b/qt/aqt/main.py index 174e67bd6..6b44da9b1 100644 --- a/qt/aqt/main.py +++ b/qt/aqt/main.py @@ -148,7 +148,7 @@ class AnkiQt(QMainWindow): def setupUI(self) -> None: self.col = None self.setupCrashLog() - self.disableGC() + self.disable_automatic_garbage_collection() self.setupAppMsg() self.setupKeys() self.setupThreads() @@ -1090,10 +1090,8 @@ title="%s" %s>%s""" % ( self.maybeEnableUndo() def autosave(self) -> None: - saved = self.col.autosave() + self.col.autosave() self.maybeEnableUndo() - if saved: - self.doGC() # Other menu operations ########################################################################## @@ -1256,6 +1254,8 @@ title="%s" %s>%s""" % ( self.progress.timer(10 * 60 * 1000, self.onRefreshTimer, True) # check media sync every 5 minutes self.progress.timer(5 * 60 * 1000, self.on_autosync_timer, True) + # periodic garbage collection + self.progress.timer(15 * 60 * 1000, self.garbage_collect_now, False) # ensure Python interpreter runs at least once per second, so that # SIGINT/SIGTERM is processed without a long delay self.progress.timer(1000, lambda: None, True, False) @@ -1621,22 +1621,39 @@ title="%s" %s>%s""" % ( # GC ########################################################################## - # ensure gc runs in main thread + # The default Python garbage collection can trigger on any thread. This can + # cause crashes if Qt objects are garbage-collected, as Qt expects access + # only on the main thread. So Anki disables the default GC on startup, and + # instead runs it on a timer, and after dialog close. + # The gc after dialog close is necessary to free up the memory and extra + # processes that webviews spawn, as a lot of the GUI code creates ref cycles. - def setupDialogGC(self, obj: Any) -> None: - qconnect(obj.finished, lambda: self.gcWindow(obj)) + def garbage_collect_on_dialog_finish(self, dialog: QDialog) -> None: + qconnect( + dialog.finished, lambda: self.deferred_delete_and_garbage_collect(dialog) + ) - def gcWindow(self, obj: Any) -> None: + def deferred_delete_and_garbage_collect(self, obj: QObject) -> None: obj.deleteLater() - self.progress.timer(1000, self.doGC, False, requiresCollection=False) + self.progress.timer( + 1000, self.garbage_collect_now, False, requiresCollection=False + ) - def disableGC(self) -> None: + def disable_automatic_garbage_collection(self) -> None: gc.collect() gc.disable() - def doGC(self) -> None: + def garbage_collect_now(self) -> None: + # gc.collect() has optional arguments that will cause problems if + # it's passed directly to a QTimer, and pylint complains if we + # wrap it in a lambda, so we use this trivial wrapper gc.collect() + # legacy aliases + + setupDialogGC = garbage_collect_on_dialog_finish + gcWindow = deferred_delete_and_garbage_collect + # Crash log ########################################################################## diff --git a/qt/aqt/stats.py b/qt/aqt/stats.py index 04158ae6c..a031b9ba2 100644 --- a/qt/aqt/stats.py +++ b/qt/aqt/stats.py @@ -27,7 +27,7 @@ class NewDeckStats(QDialog): def __init__(self, mw: aqt.main.AnkiQt) -> None: QDialog.__init__(self, mw, Qt.Window) - mw.setupDialogGC(self) + mw.garbage_collect_on_dialog_finish(self) self.mw = mw self.name = "deckStats" self.period = 0 @@ -108,7 +108,7 @@ class DeckStats(QDialog): def __init__(self, mw: aqt.main.AnkiQt) -> None: QDialog.__init__(self, mw, Qt.Window) - mw.setupDialogGC(self) + mw.garbage_collect_on_dialog_finish(self) self.mw = mw self.name = "deckStats" self.period = 0