572489 - Cached calendar refresh can cause database table is locked error

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Simon Vaillancourt]

This bug is logged to track an issue that is now much easier to reproduce since bug 466686 has been fixed. If calStorageCalendar.getItems or calStorageCalendar.getItemById is called when a provide is adding items to the local cache (assumed to be done from another thread), "database table is locked" errors start appearing in the error console.

This can be easilly reproduced by:
1) Enabling the cache for a CalDAV calendar
2) Upgrading from an old db

Also seen in the wild but harder to repro:
3) Triggering a refresh while the db is being written to

error sample:
Error: Message: Error selecting non recurring events by range!
Last DB Error Message: database table is locked

Note that before bug 564268 was fixed, this bug could also end up logging this error:
DB error: library routine called out of sequence

Comment 1

[Wolfgang Sourdeau]

Here is what happens (at least with webdav sync)

Let's say we have a few calendars with may events in them.
At launch time, specifically when the cache has not been initialized, the changelog is "replayed". For each calendar, the list of new/modified/deleted etags is queried. Once this is done, the appropriate events are fetched, during a second query.
With many large calendars, those queries will tend to overlap, meaning that the transaction count will raise 1, 2 or 3 times per calendar and while some events are being inserted others are queried (calStorageCalendar.js: adoptItem). When a SELECT is performed when only one calendar is in transaction mode, no problem occurs. However, when one occurs when another calendar is, then the exception is raised.

I used to think that modifying cal.postPone in calUtils.jsm for using the main thread rather than "currentThread" was helpful but it's actually not.

What is, however, is to avoid acquiring a transaction for batch mode, leaving only the transaction querying that occurs when inserting records. In that case the transaction count never raises above 1 which therefore enables querying from other calendars.

Now, I am wondering if this would not be a side-effect of cal.postPone, since a priori the XML parser used in calDavRequestHandlers.js is synchronous (or is it?)...

Comment 2

[Wolfgang Sourdeau]

Btw, it's useful to note that the transaction occurring in batch mode is mainly for improving performance on Windows, where a long chain of BEGIN and COMMIT is very long.

Comment 3

[Wolfgang Sourdeau]

Futher obervations that confirms the above:

1. The big difference between 0.9 and 1.0 is that transactions used to be acquired and released in batch mode only, whereas it's also the case now for every write/delete calendar operations. This change is a very good change in itself, since it respects more the purpose of transactions and which enables the rollbacking when db error occurs. The bad thing here is that this change was not complete because of the pseudo transaction nesting that occurs in batch mode.
2. With the help of calIThreadManager.isMainThread, I managed to observe that everything was run from the main thread, albeit asynchronously. This means that every write and delete is atomic from a db perspective, further rendering the transaction nesting obsolete as well as cal.postPone from the calStorageCalendar perspective.

Therefore batch mode no longer needs transactions anymore. Therefore transaction counters can be removed from the code and getItems no longer need to make use of cal.postPone.

Therefore I will produce a patch in a few minutes.

Comment 4

[Wolfgang Sourdeau]

Attachment: changes described in comments

This patch implements the changes above. Please don't mind the whitespace removal here and there (courtesy of js2-mode for Emacs).

Comment 5

[Wolfgang Sourdeau]

Please note that the above patch also poorly fixes a transaction nesting problem in flushItem. deleteItemById should simply not acquire transactions, it's the caller that should. A bug should therefore be opened whenever this patch is applied because this will cause a loss of data in the case a problem occurs during the insert.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 461540 [details] [diff] [review]
changes described in comments

>-        let this_ = this;
>-        cal.postPone(function() {
>-                this_.getItems_(aItemFilter, aCount, aRangeStart, aRangeEnd, aListener);
>-            });
As discussed, lets keep it for now. Could you file the followup bug?



>     startBatch: function cSC_startBatch() {
>-        this.acquireTransaction();
>         this.__proto__.__proto__.startBatch.apply(this, arguments);
>     },
>     endBatch: function cSC_endBatch() {
>-        this.releaseTransaction();
>         this.__proto__.__proto__.endBatch.apply(this, arguments);
>     },

These functions now do nothing other than call the superclass method. You can remove them here.

>============================================================
>--- ChangeLog	4cf964c87c21578a30c45d24dc0e018e06d26e84
>+++ ChangeLog	a9235e2b96fab5cf424d0654f3e055d72a4f87d0
Not our file :-)


I'd appreciate a new patch for easier checkin.

Comment 7

[Wolfgang Sourdeau]

Attachment: new patch

New patch with your comments taken into account.

Comment 8

[Wolfgang Sourdeau]

Additional issues filled:
- transaction issue in flushItem: #583278
- postPone and exception in calendar-multiday-views.xml: #583279

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/41c0dab1b83a>
-> FIXED

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/0c967a05fe2e>

a=philipp