Cached calendar refresh can cause database table is locked error

RESOLVED FIXED in 1.0b3

Status

Calendar
General
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Simon Vaillancourt, Assigned: Wolfgang Sourdeau)

Tracking

Lightning 1.0b1
1.0b3

Details

(Whiteboard: [needed chemspill])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
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?)...
(Assignee)

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
Created attachment 461540 [details] [diff] [review]
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).
Attachment #461540 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #461540 - Flags: review? → review?(philipp)
(Assignee)

Comment 5

8 years ago
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 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.
Attachment #461540 - Flags: review?(philipp) → review+
Whiteboard: [needed chemspill]
(Assignee)

Comment 7

8 years ago
Created attachment 461569 [details] [diff] [review]
new patch

New patch with your comments taken into account.
Attachment #461540 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #461569 - Attachment description: new path → new patch
(Assignee)

Comment 8

8 years ago
Additional issues filled:
- transaction issue in flushItem: #583278
- postPone and exception in calendar-multiday-views.xml: #583279
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/41c0dab1b83a>
-> FIXED
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Assignee: nobody → WSourdeau
OS: Windows XP → All
Hardware: x86 → All
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/0c967a05fe2e>

a=philipp
You need to log in before you can comment on or make changes to this bug.