Opened 10 years ago

Closed 10 years ago

#431 closed enhancement (fixed)

Add Notifier trigger hook to DB.commitTransaction() and queue notifications during transactions

Reported by: dstillman Owned by: dstillman
Priority: major Milestone: 1.0 Beta 3
Component: data layer Version: 1.0
Keywords: Cc: simon, stakats

Description

I'd like to make a fairly major change to the way Notifier triggers are handled.

At the moment triggers run whenever they are called, which creates a number of problems:

1) It slows down data operations for unnecessary UI updates.

2) To get around #1, Simon (I believe) turns off most notifications during translation and triggers them at the end manually. I group ids during long operations and trigger them at the end. This requires keeping track of what items to notify and passing them through various chained methods.

3) Since #2 is tedious and sometimes impossible when methods/transactions are nested, triggers are sometimes called unnecessarily for the same item, which slows things down further.

4) #3 will also cause utilities to make unnecessary updates to remote websites, slowing things down even more.

5) Most importantly, if triggers are called before transactions complete, the UI will reflect data that hasn't been safely committed to the DB. This is especially problematic if a nested transaction without proper rollback procedures (which are easy to overlook) fails in the middle--a user can in fact keep using Zotero with it in the middle of a transaction the entire time, since the UI is still updating. To be safe (after one or two reports of people losing entire sessions' worth of data), I added a shutdown check that writes out open transactions, but that's a dangerous approach for a number of reasons.

What I'd like to do is have Notifier.trigger() queue the trigger events if a DB transaction is in progress and have the queue be run all at once by a callback in DB.commitTransaction(). Redundant ids will be ignored, UI updates and network traffic will be consolidated, and the UI will reflect what's actually in the DB, so if there's an uncaught exception in the middle of a transaction, the UI will just break and the user will restart Firefox.

(It also occurs to me that the Notifier should probably have its own begin() and commit() functions that can be used independently of the DB ones (and in place of the enable/disable methods), since translation code, for example, might be asynchronous enough that there's not a single DB transaction throughout the entire process. It would probably still benefit from having the Notifier do its own queueing, though. Simon, is that correct?)

Anybody foresee any problems with this approach? I'm going to try to implement it later today if there are no objections.

Change History (1)

comment:1 Changed 10 years ago by dstillman

  • Resolution set to fixed
  • Status changed from new to closed

(In [928]) Closes #431, Add Notifier trigger hook to DB.commitTransaction() and queue notifications during transactions

unlock = Zotero.Notifier.begin(lock)
Zotero.Notifier.commit(unlock)
Zotero.Notifier.reset()
Zotero.DB.addCallback(type, callback)

begin(), commit() and reset() are added to beginTransaction(), commitTransaction() and rollbackTransaction(), respectively, on startup, so notifications are now automatically queued during DB transactions -- this has the potential to make complex operations dramatically faster

begin() can also be called manually -- pass true to indicate that intermediate commit()'s (e.g. called from commitTransaction()) shouldn't run the event queue, and pass the value returned to the matching commit() call. (The return value of commit() will be true if it is the first begin() to request a lock and false otherwise -- this allows multiple begin(true) calls to be nested without the nested ones triggering notification.

This does make trigger() order a bit less predictable, but I'm ordering events and types (e.g. calling modify events after add and before delete) in an attempt to avoid problems. We'll see if this works or we need a more sophisticated ordering/grouping scheme.

Note: See TracTickets for help on using tickets.