756052 - Adapt offline interfaces to avoid extra methods for providers

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Attachment: WiP - v1

To make it easier for providers implementation, it would be nice to avoid the *OrUseCache methods.

This patch is work in progress, but moves most of the repeating implementation to calCachedCalendar. I'd like some feedback on this before I continue.

Things left to do, please comment if needed:

* Find out why the add method requires the provider to take over the add in the cached case. Can't we have the cache layer do it?

* Look for a few more status codes denoting network errors and accept them in the cache layer as codes that should trigger offline add/modify/delete.

* Make sure deleting from the cache is working as expected.

Comment 1

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

> * Find out why the add method requires the provider to take over the add in
> the cached case. Can't we have the cache layer do it?
This may sound ambiguous. I mean that for cached calendars, the provider is forced to do the add to the offlineStorage in the calCachedCalendar listener. This wasn't the case for all other methods. Why?

Comment 2

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

Oh one more point:

* Find out why the onOperationComplete listeners are not called on add/modify/delete errors.
  - This was introduced over 4 years ago in bug 393817
  - Fixing this might actually get rid of some "views no longer work after ... " bugs

Comment 3

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

* Figure out if there are endless loops of adding items to the cache if the server
  replies with a 5xx status code
  - Not sure how to best test this, probably injecting fake code.

Comment 4

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

Attachment: Reduce needed methods - v1

Ok, I've taken care of all mentioned points. I've put the listener changes into a separate patch, since its kind of a different bug, but since the first patch makes the listeners on error required, we better push these two together.

I'm open to any comments, please test and review :-)

Comment 5

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

Attachment: Fix CalDAV Listeners - v1

Comment 6

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

Maybe a note on the listeners patch: I've decided to remove all notifications if the calendar is cached, since the items will end up in the offline cache in that case. This is just the first step of getting all the direct error notifications out of the provider, more to come in different bugs.

Comment 7

[Mohit Kanwal [:redDragon]]

(In reply to Philipp Kewisch [:Fallen] from comment #0)
> Created attachment 624698 [details] [diff] [review]
> WiP - v1
> 
> To make it easier for providers implementation, it would be nice to avoid
> the *OrUseCache methods.
> 
> This patch is work in progress, but moves most of the repeating
> implementation to calCachedCalendar. I'd like some feedback on this before I
> continue.
> 
> Things left to do, please comment if needed:
> 
> * Find out why the add method requires the provider to take over the add in
> the cached case. Can't we have the cache layer do it?

The sequence of operations is something like this during the add method: the adoptItem checks if the application is offline, and adds offline flag and adds it to the cache. 

Now when the application comes back online, if the underlying calendar exposes addItemOrUseCache methods then the function calls up it with useCache set to false so that if there is some network failure of any kind then let the provider handle the case/it might add it to the cache (where there are 2 cases again :- it might be the same item that had an offline addition flag attached to it, which is the reason why there are doAddItemOrUseCache methods having useCache set to either true or false called from within the provider file or it can be a completely new item) 

The *OrUseCache methods provide a signal to the provider what to trust (false means trust the cache, true means don't trust the cache and add to cache it) when there is some problem. 

One way we thought was to do it via the cache layer using error codes, but then the problem was in the case of addition specifically there would be a loop, let's say you added an item in the offline mode, it would have OFFLINE_ADDITION_FLAG in the sqlite db, and let's assume the callback is going to handle any cases of problems related to network codes, but then if the network error happens every time you try to connect to the server it is going to send the same error code back to the cache. So at least the cache needs to know which version of the item is the true copy (or at least the provider). This is the reason why it was necessary to include the *OrUseCache methods in the interface file because, in the true addition case, when there is no error we can set useCache = true and override it while when doing the replayChangesOn we can set useCache = false and nothing happens to the item in the cache, expect that it flag gets removed and the server receives the copy of the item. 

It is complicated, I shall try to explain the different possibilities through a table of some sort. 

Case#1.Addition of items
[offlineItem below means, item exists in the cache but not on the server]

Type of Item 	|	Offline   |  Network Error| Behaviour
-----------------------------------------------------------
1. 	newItem			Y				Y		Item added to cache with flag  
2.  offlineItem		Y				Y		Nothing happens (cache layer code)
3.  newItem			N				Y		Item added to cache see [3]
4.  offlineItem		N				Y		Nothing to be done see[4] (replay mode)
5.  newItem			N				N		Normal behavior
6. 	offlineItem		N				N		Successful remove flag(replay mode)
7. 	newItem			Y				N		Offline Mode Code (cache)
8.  offlineItem		Y				N		Offline Mode Code (cache)(replay mode)						 

comments
--------
[3] The cached layer calls up *OrUseCache method with useCache = true, essentially telling to the provider that the cache can be ignored, overwritten (essentially the item does not exist)
[4] The cached layer calls up *OrUseCache method with useCache = false, telling to the provider that trust the cache object and don't overwrite, even if there is a failure or network error of some sort. This is done via the replayChangesOn method which plays back offline added items and since it sets useCache = false preventing the provider from sending a success code 

The problem that I find is that the provider will send the success code if it did not encounter any network error (case 5 above) and in that case the cache layer will add it, but if the item exists then the flag must be removed.

So multiple invocations of the network error code (same error code) will cause troubles we would need to know if the item exists in the cache beforehand.

> 
> * Look for a few more status codes denoting network errors and accept them
> in the cache layer as codes that should trigger offline add/modify/delete.
> 
> * Make sure deleting from the cache is working as expected.

Jeez, that's long! sorry for taking a long time, lemme figure out whether the patch works. I need to get my head around the different code changes.

50X errors can be simulated by using Sogo's CALDAV server VM and turning off the httpd process.

Comment 8

[Mohit Kanwal [:redDragon]]

Comment on attachment 625640 [details] [diff] [review]
Reduce needed methods - v1

Review of attachment 625640 [details] [diff] [review]:
-----------------------------------------------------------------

I have figured out some issues while trying to deal with the code, applied the patch, tested using Sogo virtual appliance which hosts some test caldav, ics (read-only) calendars for use. Error 50x can be simulated by turning off httpd service in the Sogo console. 

There is one issue of looping happening whenever replaying of items happens when the server is in 50x mode, and error is thrown coz item of same id exists. I mean we can catch this error, but that's a kind of a hack in my notion. 

For the deletion and modification case there is no problem as the offline_flag is handled by the storage layer and it can safely assume that the item exists in cache whether it is offline mode or replay mode, the contents of the item won't change, even if there is looping. It is the addition case which we have to look into.

Maybe we discuss in detail about this in one of the IRC sessions.

The listeners part is left. I will test that later today.

::: calendar/base/src/calCachedCalendar.js
@@ +537,4 @@
>                          addListener.itemCount = this.items.length;
>                          for each (let item in this.items) {
>                              try {
> +                                this_.mUncachedCalendar.addItem(item, addListener);

This is replay code, the item replayed has an offline flag, Assuming the calendar is in online mode. If there is a network error then this goes to the offline addition code, assuming the provider does not handle this situation and throws an error back at the cached layer. It will cause problem. The cache will think it is a legitimate item failing addition due to server fault. There must be a way of signaling the provider that it needs to raise an error based on the updated item already being present in the cache. And hence we have the endless loops of 50x errors causing problems.

Another way which I think now is to manually check the flag of the item in the cached layer, but that has been right now left to the storage layer.

@@ +603,5 @@
>                          modifyListener.itemCount = this.items.length;
>                          for each (let item in this.items) {
>                              try {
> +                                // The calendar supports the changelog functions, let it modify the item
> +                                // TODO is it ok to not have the old item here? Pass null or the new item?

The uncached provider layers need 2 items: old and new, it throws an error if the old item is null. Now when the item is in the offline cache, we dont have a copy of the old item. So therefore we just pass item.

@@ +762,5 @@
> +                if (isUnavailableCode(status)) {
> +                    // The item couldn't be added to the (remote) location,
> +                    // this is like being offline. Add the item to the cached
> +                    // calendar instead.
> +                    self.adoptOfflineItem(item, listener);

Here the code is going to add to the cache, if let's say the network error handling was done by the cached layer then it needs to first check if item exists in cache or not.

@@ +804,5 @@
>      modifyItem: function(newItem, oldItem, listener) {
> +        let self = this;
> +
> +        // First of all, we should find out if the item to modify is
> +        // already an offline item or not.

Correct approach here but I think we need to extend this checking of flag to add item and the replay cases.

@@ +887,5 @@
> +        // First of all, we should find out if the item to delete is
> +        // already an offline item or not.
> +        let flagListener = {
> +            onGetResult: function() {},
> +            onOperationComplete: function(c, s, o, i, offline_flag) {

expanded notation for variables.

::: calendar/providers/caldav/calDavCalendar.js
@@ +822,4 @@
>              }
>  
>              // 204 = HTTP "No content"
> +            // 404 = Not Found - This is kind of a success, since the item is already deleted.

404 can also be raised when the calendar does not exists?

Comment 9

[Mohit Kanwal [:redDragon]]

You can download sogo zeg image from here: http://www.sogo.nu/english/downloads/zeg.html and mount it in oracle virtual box.

Comment 10

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

I honestly don't know where to start :-} Let me comment on a few selected issues:

(In reply to Mohit Kanwal [:redDragon] from comment #8)


> @@ +762,5 @@
> > +                if (isUnavailableCode(status)) {
> > +                    // The item couldn't be added to the (remote) location,
> > +                    // this is like being offline. Add the item to the cached
> > +                    // calendar instead.
> > +                    self.adoptOfflineItem(item, listener);
> 
> Here the code is going to add to the cache, if let's say the network error
> handling was done by the cached layer then it needs to first check if item
> exists in cache or not.
So what should be different when the item already exists? The storage calendar acting as the cache is set to relaxedMode, so adding an item that already exists will just overwrite the item.

> > +            onOperationComplete: function(c, s, o, i, offline_flag) {
> 
> expanded notation for variables.
I left these single-character, since they are not used.

> >              // 204 = HTTP "No content"
> > +            // 404 = Not Found - This is kind of a success, since the item is already deleted.
> 
> 404 can also be raised when the calendar does not exists?
Then things will be failing much earlier, afaik. We don't have a way to differ between those cases anyway.

Maybe we can schedule an IRC meeting or even phone meeting to discuss the rest?

Comment 11

[Mohit Kanwal [:redDragon]]

Comment on attachment 625641 [details] [diff] [review]
Fix CalDAV Listeners - v1

Review of attachment 625641 [details] [diff] [review]:
-----------------------------------------------------------------

Looks alright to me, I have tested it with the different combinations of server status and lightning online-offline states and have found no issues. Good to go from my side. 
Cheers

Comment 12

[Mohit Kanwal [:redDragon]]

Comment on attachment 625640 [details] [diff] [review]
Reduce needed methods - v1

Review of attachment 625640 [details] [diff] [review]:
-----------------------------------------------------------------

Have tested this patch against the different possibilities of server config, lightning status and can say that it works functionally well. Previously when I had tested with a CALDAV calendar and SOGo's ZEG virtual appliance (for simulating the CALDAV calendar locally) I was getting an error raised (Item cannot be modified DAV_ERROR) when an offline added item was being sent to the server which was under maintenance (error code 503). Looks like it is gone now, i am still wrapping my head around it, have tested that particular situation multiple times and can't seem to reproduce it. For the other scenarios the extension works as expected.

So I think this patch is good to go now, soon wcap can also avail the offline cache facility and benefit from it. Looking forward to the patch :-) !

Philipp has opened bug 778532 to document the cache feature which will provide information for future providers how to integrate with the cache and its inner workings.

thanks Philipp for the hard work :) ! 

cheers

::: calendar/providers/caldav/calDavCalendar.js
@@ +611,5 @@
>                  cal.LOG("[calDavCalendar] Server unavailability code received. Items are being put into cache for a later try");
> +                aListener.onOperationComplete(thisCalendar.superCalendar,
> +                                              Components.results.NS_ERROR_NOT_AVAILABLE,
> +                                              Components.interfaces.calIOperationListener.ADD,
> +                                              parentItem.id,

In the other patch Components.interfaces.calIOperationListener.ADD is denoted by cIOL.ADD in some places, is it done here on purpose? or can it be replaced by cIOL.ADD?

Comment 13

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

comm-central changeset ccab1c205bb0 (fix caldav listeners)
comm-central changeset 7aac2e3b3ef0 (reduce needed methods)