456208 - Disabling calendars autorefresh does not prevent cached calendar sync

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[klint]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 Creative ZENcast v2.00.14
Build Identifier: Lightning 0.9 20080917 + Gcal Provider 0.5 20080917

I have set the calendars not to be refreshed automatically, in the Lightning options (to avoid too often high CPU consumption)
But the cached calendar keep on refreshing/synchronizing every 4 minutes, although they should not.

Reproducible: Always

Steps to Reproduce:
1. Set the "Refresh frequency" to null (refresh manually) in the Lightning options
2. Create a cached remote calendar
Actual Results:  
The cached calendar is synchronized every 4 minutes.

Expected Results:  
The cached calendar should be synchronized (at least from remote-to-local) only when the remote calendars are manually refreshed

More generally, shouldn't the cached calendar synchro frequency be equal to the global calendars refresh frequency? or at least somehow related?

Additional minimal proposal: allow the cache synchro frequency to be set via a Lighting preference in about:config.


This bug is very important IMHO, as, considered together with the fact that the synchro is highly CPU consuming, it hangs Thunderbird every 4 minutes. This is very annoying when you write a lot of emails at work, for instance, as it often suspends keyboard entry for some seconds right in the middle of composing a new message (up to 30s depending on your calendar's size)

Comment 1

[Daniel Boelzle [:dbo]]

Just to add: This issue is not about ics subscriptions, but providers supporting changelog, e.g. caldav, gdata or wcap.

Comment 2

[Stefan Sitter [:ssitter]]

Have you also disabled the automatic check for new invitations via the advanced preferences "calendar.invitations.autorefresh.enabled" and "calendar.invitations.autorefresh.timeout"?

Comment 3

[klint]

yes, Stefan, I have. It really seems to be a problem with cached calendar sync, not with invitations discovery.

Comment 4

[Chuck]

I am experiencing the same problem with this bug. It is so bad that I can't stand to keep TB running anymore, and I am seriously considering migrating to another mail client, if this doesn't get fixed soon. It has been happening for weeks now. My CPU usage spikes for at least 30 seconds and locks up TB completely. It is such a bother, that it has rendered the program almost useless. Somebody please help!

Comment 5

[Chuck]

I am experiencing the same problem with this bug. It is so bad that I can't stand to keep TB running anymore, and I am seriously considering migrating to another mail client, if this doesn't get fixed soon. It has been happening for weeks now. My CPU usage spikes for at least 30 seconds and locks up TB completely. It is such a bother, that it has rendered the program almost useless. Somebody please help!

Comment 6

[Stefan Sitter [:ssitter]]

Chuck, why not disable the experimental (!) cache feature if it bothers you?

Comment 7

[Chuck]

I would love to, Stefan, if I had any clue how to do such a thing.
If it will act as a temporary fix, then someone please tell me how to perform it.

Comment 8

[klint]

Chuck, just edit your calendars' properties in Lightning and uncheck the "Use Cache" checkbox.

But you may also want to switch (temporarily) from Lightning to Sunbird: move all your calendar subscriptions to Sunbird, activate the caching mechanism in Sunbird and you will not suffer anymore from Thunderbeing being frozen every 4 minutes, but you still will have cached calendars. Of course, you would loose the tighter integration of thunderbird and lightning.
Personnally, I have chosen the second option until this issue is solved in Lightning.

Comment 9

[Chuck]

I just looked, and the experimental cache option has been unchecked the whole time. I am not going to go through all the work of migrating to Sunbird, just to migrate back when the bug is fixed. I sure as hell hope somebody pays enough attention to this problem to fix the thing, because it's a doozy. I can't believe that the update was released with such a serious and obcious bug in it.

Comment 10

[klint]

If your cache is not enabled, maybe you're suffering from another bug. Check bug 449449 and apply the workaround explained there.

Comment 11

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

I think we should make this a hidden pref, not a per-calendar thing. I could also live with it defaulting to a hidden pref and being a per-calendar thing.

Comment 12

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

Attachment: Fix - v1

This patch uses the autorefresh interval for all calendars, unless the hidden pref is set. The value is read in the following order:

* per-calendar basis (calendar.registry.{uuid}.cache.updateTimer)
* default value (calendar.cache.updateTimer)
** If updateTimer -1 then take(calendar.autorefresh.timeout)
*** If autorefresh is 0, then default to 4

Comment 13

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

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

Comment 14

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

Comment on attachment 449516 [details] [diff] [review]
Fix - v1

Well, actually, this doesn't really fix the bug, this only adds the new preference. There is still no way to totally disable the cached calendar sync.

The problem is, we have no UI that allows manually refreshing the cached calendar. I'll need some more time to debug, then I'll upload a new patch.

Comment 15

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

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

Comment 16

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

This is of course not pushed, the comment was a side-effect of trying push-to-try with my bugzilla extension enabled.

Comment 17

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

Attachment: Fix - v2

Last I said we have no UI to trigger the cache update, but it seems this changed. When I select "reload remote calendars" it seems that a cached calendar sync is also triggered. This patch takes care by actually disabling the autorefresh timer if it has the same value as the autorefresh timeout (otherwise there will be two refreshes), and also disables the timer if the value is 0 or autorefresh is disabled.

Comment 18

[Simon Vaillancourt]

Comment on attachment 470540 [details] [diff] [review]
Fix - v2

Hello,
Here is my review:
1) Add an observer on calendar.autorefresh.enabled, otherwise if you have it disabled and re-enable it, it will not get picked up(don't forget the removeObserver): 
Services.prefs.addObserver("calendar.autorefresh.enabled", this, false);

2) The reset() function does not work, from my tests a cached provider should always return false to "canRefresh" (see caldavProvider implementation) so doing "if (autorefresh != timeout..." is actually breaking the refresh of cached calendars if they happen to have the same positive value.

3) It seems that if updateTimer is set as a property or a preference to something other than -1, it will override the preference  calendar.autorefresh.enabled=false, ie: even if autorefresh is false, the calendars will get refreshed , I think it should not be the case.

r=simon with the above changes

Nitpick:
I think that "autorefresh" and "timeout" are very confusing, could we rename them to something better, I personally like extremely verbose names so I will remember what they do in 6 months :) ?  for example:
autorefresh => CalendarAutoRefreshIntervalInMin
timeout => CacheAutoRefreshIntervalInMin

Comment 19

[Martin Schröder [:mschroeder]]

Comment on attachment 470540 [details] [diff] [review]
Fix - v2

Simon has given r+ in comment 18

Comment 20

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

Attachment: Fix - v3

I've discussed with Simon that we want to change the autorefresh/updateTimer preferences a bit. We now allow setting this preference per calendar. This means we only need one timer per calendar, instead of the global autorefresh in addition to the updateTimer.

Comment 21

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

Attachment: Screenshot

Note the UI of the patch is currently horrible. I couldn't come up with something that fits well, I'd appreciate some input.

Comment 22

[Andreas Nilsson (:andreasn)]

Comment on attachment 503447 [details]
Screenshot

In general I like the screenshot and the wording is good.
Some small issues:
* Refresh Calendar should align to the topmost radiobutton instead of being centered in between in the radiobuttons.
* The Read Only, Show Alarms etc. items looked like they belonged to the Calendar refreshing at first, so I would either increase the spacing between those and the items above, use a line or put the Refresh Calendar stuff at the bottom of the dialog.

Comment 23

[Simon Vaillancourt]

Attachment: Patch with alternative ui

Here is my counter proposal for the ui change, it is a bit more restrictive but much simpler IMO. It resembles what Apple iCal does.   Note that a advanced user wanting a refresh interval different than the pre defined one can always set it using the config editor.

This patch includes Philipp's patch with my ui changes applied on it.

Comment 24

[Simon Vaillancourt]

Attachment: Sample of the new simplified calendar refresh edit

Comment 25

[Simon Vaillancourt]

Comment on attachment 503446 [details] [diff] [review]
Fix - v3

simon=r  but I ask Philipp to consider my ui proposal as well and decide which one he likes the most.

Comment 26

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

Comment on attachment 508415 [details] [diff] [review]
Patch with alternative ui

Well ok, I guess we can do it this way :)

Unfortunately, I totally overlooked the fact that you've already provided a patch and created one too. Sorry about that!

>+            <menuitem hidden="true" id="calendar-refreshInterval-custom-menuitem" label="&calendarproperties.refreshInterval.auto.prefix.label; !@custom!@ &calendarproperties.refreshInterval.auto.postfix.label;"/>
>+            <menuitem label="&calendarproperties.refreshInterval.auto.every.minute.label;" value="1"/>
>+            <menuitem label="&calendarproperties.refreshInterval.auto.prefix.label; 5 &calendarproperties.refreshInterval.auto.postfix.label;" value="5"/>
>+            <menuitem label="&calendarproperties.refreshInterval.auto.prefix.label; 15 &calendarproperties.refreshInterval.auto.postfix.label;" value="15"/>
>+            <menuitem label="&calendarproperties.refreshInterval.auto.prefix.label; 30 &calendarproperties.refreshInterval.auto.postfix.label;" value="30"/>
>+            <menuitem label="&calendarproperties.refreshInterval.auto.every.hour.label;" value="60"/>
Some languages have very complex rules (up to 10 different cases) for prefix/postfix strings of numbers. It would be better to use PluralForm.jsm and a single string in a .properties file.

If its ok with you, we could take my patch, which doesn't have the above issue. The only other difference is that it uses "Every 60 minutes" instead of "Every hour".

Comment 27

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

Attachment: Fix - v4

Comment 28

[Simon Vaillancourt]

Comment on attachment 510060 [details] [diff] [review]
Fix - v4

simon=r but with this minor change in calendar.properties, otherwise all labels in plural form start with a space:

-calendarPropertiesEveryMinute=Every minute; Every #1 minutes
+calendarPropertiesEveryMinute=Every minute;Every #1 minutes

Comment 29

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

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

Comment 30

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

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/c80f8fbe07c3>
a=philipp