Surprising 304 Handling Seems to Have Broken App #1294

Open
cfis opened this Issue Mar 23, 2013 · 15 comments

4 participants

@cfis

This commit, a9ef1fe, unfortunately seems to have broken our app.

In our app, there is a anonymous database and an authenticated database. The only difference between the two is you get to see more stuff when you login.

So when someone opens a zerista app (not logged in) one of the first things we do is make a call to the /admin/menu/native. This url returns json that defines what icons should be shown on the home screen. The response json is locally cached automatically by ios.

When a user then logs in, we call that url again. The server returns 304 (nothing has changed an anonymous and authenticated user get the same menu). This now triggers canSkipMapping, which returns from performMappingOnResponse with 0 objects. We don't have fetchRequestBlocks setup, and even if we did it would not matter since we are using a 2nd database. This mean no data gets loaded into the authenticated database and users end up with blank screens. Not so good...

So two issues:

First, if I understand the code correctly, this means that 304 handling only works if you've registered a fetch request block. That seems pretty surprising, so maybe I'm missing something?

Second, in a case like ours, we can't make it work anyway since we have switch from the anonymous database to the authenticated one.

If I'm not understanding this code, please let me know, so I can work up a solution. If I am, can this be changed so 304's work out of the box like before?

@blakewatters blakewatters was assigned Mar 23, 2013
@blakewatters
The RestKit Project member

This is a very fair point -- I will update the canSkipMapping method to check if there's a fetch request block registered. I have them in place for all of my endpoints in GateGuru so this slid right through

@blakewatters blakewatters added a commit that closed this issue Mar 24, 2013
@blakewatters blakewatters Fix issues with 304 caching when there are no Fetch Request Blocks re…
…gistered that match the URL. closes #1294
5052b61
@cfis

Thanks for looking a this Blake and providing such a quick fix. The solution fixes the immediate problem, but doesn't seem quite right since it would seemingly preclude us from being able to use the fetchRequestBlock functionality.

We aren't currently using it, but we do have to deal with removing orphaned objects, and so it seems like a good step forward (didn't know it existed til yesterday). So let's say we started using fetchRequestBlocks for the /admin/menu/native url discussed above. All would be good for an anonymous user the first time the app synced menus. As long as the user stayed logged out, any possible changes after that would also work (the menus do change and we sync them once a day - conference organizer tend to change their minds).

But the moment the user would log in, seems like we would run right back into the 304 handling problem again. We would have registered handlers that would trigger the no mapping path - but since logged in users have a different instance of the database, then fetchRequestBlock would return no objects and we would be back to our blank screen problem.

It seems that tying the functionality used to deleting orphan objects to optimizing 304 handling is coupling two separate issues that shouldn't be coupled - resulting in a less flexible library and unexpected behavior. Is performing the object remapping such a performance hit that it makes this coupling worth while? And if so, then could the fetchRequestBlocks be registered twice - once for object cleanup and once for 304 handling so that apps using RestKit could control this behavior in a way that suits their app?

Or, more directly, how could we use fetchRequestBlock to do orphan cleanup but not have it break 304 handling for us?

@blakewatters
The RestKit Project member

Thanks for the feedback and follow through on this. This is a very important area of the API and we need to get it right. I am about to head to bed shortly, so I have reopened the issue and will address your comments in depth tomorrow.

@blakewatters blakewatters reopened this Mar 24, 2013
@dhanushram
@blakewatters
The RestKit Project member

@dhanushram I think you have overlooked an important benefit of the fetch request blocks when you have 304 responses + fetched results controller: freeing up the mapping queue. Even if you are doing all your networking in the background, a 304 response that is mapped out of the cache entry is going to block the responseMappingQueue, which has a default concurrency limit of 1. This means that any other requests that may need mapping will be queued up while you are processing a bunch of redundant data. With a fetch request block enabling the response to be serviced directly from Core Data without any mapping the entire operation can complete in milliseconds.

@dhanushram
@blakewatters
The RestKit Project member

@cfis -

First off -- to answer your question about the performance benefits of the 304 optimization: it is very significant in some cases. Within GateGuru, we have a few very large global payloads (such as the list of all Airports) that change very infrequently. We use a combination of Russian Doll caching and Postgres database triggers on the server to mutate the cache keys when data within this payload gets stale. It takes several seconds to map the payload from JSON, which is a complete waste if nothing has changed. With the 304 optimization in place, it completes in a few milliseconds -- its so fast you can hardly notice it on a solid connection. This let's us just fire a request to the server whenever the Airports list loads up without worrying about the performance or trying to do scheduled syncs, pagination, etc. since you are very likely to get a cache hit. In many cases, if you have an optimized web backend returning non-trivial payloads then the mapping phase is going to take much longer than the HTTP transport. Mileage will obviously vary depending on the size of the responses you are shoveling around, but as I mentiomed to @dhanushram yesterday skipping the mapping keeps the response mapping queue available, which makes other requests spend less time waiting for service.

With that said, I've done a bit of thinking on your case and few ideas come to mind in order of least framework changes to most:

  1. Update Zerista to expire the [NSURLCache sharedCache] whenever the user's authentication status changes, effectively discarding any cache entries and making everything get remapped.
  2. Rework the cache entry flagging such that it keeps track of the persistent store into which the response was mapped instead of just a boolean flag. This would disambiguate it between your persistent stores and trigger mapping on authentication changes.
  3. Rework the API's for the fetch request blocks for more flexibility. We could do something like:
typedef enum {
  RKFetchRequestBlockModeCleanup,
  RKFetchRequestBlockModeFetch
} RKFetchRequestBlockMode;

That would indicate the purpose of the fetch request being requested and let you customize or decline on a case-by-case basis.

@cfis

Hi Blake,

Thanks for the explanation on the caching - makes sense.

Not to get too off topic, but I think this question affect the api design - does fetchRequestBlock work with paging? It sounds like you are pulling all data down for a particular service at once - and from looking at the implementation code that seems to be a requirement for fetchRequestBlock to support orphan detection/deletion.

In our case, we can have anywhere from 100 to 40,000 attendees, 10 to 5,000 exhibitors or 5 to 25,000 sessions (yes, really). So our api is designed around grabbing pages of data at a time since clearly we can't get it all at once (aside - when we actually have 40k users we actually create a prebuilt core data db on the server side, gzip it, and send it down to the client to avoid all this paging/mapping - one of the reasons for the anonymous version and authenticated version of the local db).

Bringing it up since it might make one of the proposed options better than another.

Onto your suggested solutions:

  1. Expire the [NSURLCache sharedCache] whenever the user's authentication status changes. Yes, I thought the same thing and verified that it would work. Its not the greatest solution though in that we get no benefit from 304 responses (at least can avoid downloading data again since the client already has it).

  2. Rework the cache entry flagging. That would work - although I don't want to force an akward API onto RestKit for our unusual use case (or maybe someone else does something like this too?).

  3. Rework the API's for the fetch request blocks for more flexibility. I like this idea the best - doesn't force RestKit to support our use case but hopefully would allow us enough flexibility to accomplish what we need.

Charlie

@blakewatters
The RestKit Project member

The pagination use-case is interesting to me, but we currently don't do pagination within GateGuru so its not something I have hit as hard. The fetch request blocks can in theory handle it as you get access to the full URL, but how you could accurately predict what page a given object belongs to with an offset based approach is not apparent to me. If things were broken up alphabetically or something it seems tractable. I think with a paginated collection you pretty much cannot use the orphaned object cleanup or the 304 fetching optimization unless you were to spider the entire collection and consider it in aggregate.

An approach that would should work for handling orphaned object cleanup in a paginated situation would be to emit HEAD requests for each item in the collection and delete any that return a 404 (Not Found) or 410 (Gone) status code. You'd still wind up remapping each item in the collection form the cached response, but at least the database would be scrubbed.

An idea that I've been toying with that might be interesting for accelerating both cache hits and partial cache hits (i.e. something changed in the collection, but not necessarily everything) would be to designate a checksumAttribute (better name?) that is checked before any mapping is performed if the checksum attribute matches the current value in the database, the mapper skips over processing the properties of that node in the payload and keeps trucking. We'd have to measure it, but my guess is that there is a decent performance win there. For Rails you could use updated_at provided that you hook up touch events for any models that are sent nested within the payload.

@valeriomazzeo
The RestKit Project member

just as a quick note... it still seems to be something wrong on this.

We started to have some caching update problem in our app since we updated to the master version of restkit (and then the latest development, same behaviour).

inside canSkipMapping this two variables:

  • cachedEtag
  • responseEtag

at some point for some reason they always have the same value, even if the resource remotely has changed and the etag of the new response is different.

it was 100% working on the old release. Our submodule was pointing to this commit 0166576 before we moved it to the master.

@blakewatters
The RestKit Project member

@valeriomazzeo Can you provide some insight into the Cache-Control headers that you are using? Do you have revalidation configured?

@valeriomazzeo
The RestKit Project member

this is the header that is sending to restkit in the response:

Status Code: 200 OK
Accept-Ranges: bytes
Connection: Keep-Alive
Content-Length: 2235
Content-Type: application/json
Date: Wed, 27 Mar 2013 08:56:50 GMT
Etag: "135d2d0-8bb-4d8e42b73e000"
Keep-Alive: timeout=15, max=100
Last-Modified: Wed, 27 Mar 2013 08:53:20 GMT
Server: Apache/2.2.22 (Unix) DAV/2 PHP/5.3.15 with Suhosin-Patch

it is just the mac os x web server on my local machine, nothing configured and it was working.

on the live server we have this header:

Status Code: 200 OK
Age: 0
Cache-Control: max-age=3600, public
Content-Length: 2175
Content-Type: application/json
Date: Wed, 27 Mar 2013 08:58:17 GMT
Etag: "20000000016159-87f-4d8d70e326178"
Expires: Wed, 27 Mar 2013 09:58:17 GMT
Last-Modified: Tue, 26 Mar 2013 17:14:34 GMT
Server: Apache
Via: 1.1 varnish
X-Cachable: YES
X-Varnish: 954566188

and doesn't work either.

@valeriomazzeo
The RestKit Project member

after further investigation I think it has something to do with NSURLCache directly, because in my cache db the old cached request is not invalidated for some reasons.

Inside AFNetworking on this method:

  • (void)connectionDidFinishLoading:(NSURLConnection __unused *)connection

self.responseData already contains the cached response (instead of the new one).

Edit:
More investigation going on, I have reason to believe that has been broken since a9ef1fe

line 316: self.cachedResponse = [[NSURLCache sharedURLCache] cachedResponseForRequest:requestOperation.request];

line 394: NSCachedURLResponse *cachedResponse = [[NSURLCache sharedURLCache] cachedResponseForRequest:request];

I guess this prevent the response to be sent to the backend that is exactly what happened.

Ex. I deleted the remote json file I was trying to load with restkit from my machine and restkit correctly loaded the cached response. I guess it's because I am not setting any kind of cache expiration so that request is supposed to be valid forever.

So, apparently everything seems to be right on AFNetworking/Restkit side, apart from one thing that I am not sure,
should RestKit consider always invalid a cached request that doesn't have any cache control instead of considering it always valid?

@blakewatters
The RestKit Project member

@valeriomazzeo Thanks for the headers. NSURLCache keeps track of all requests regardless of cache headers, so you are probably correct that I am working with cached responses that should be excluded. I'll get some tests together around other cache control configurations and get this straightened out.

@manggitng manggitng added a commit to manggitng/RestKit that referenced this issue Apr 12, 2013
@blakewatters blakewatters Fix issues with 304 caching when there are no Fetch Request Blocks re…
…gistered that match the URL. closes #1294
9e08a9d
@magnus-staberg magnus-staberg added a commit that referenced this issue May 1, 2013
@blakewatters blakewatters Fix issues with 304 caching when there are no Fetch Request Blocks re…
…gistered that match the URL. closes #1294
8f22bb1
@blakewatters
The RestKit Project member

Bumping this forward. I am not ready to pull the trigger on any API change to the fetch request blocks for 0.20.1

@stojanovicigi stojanovicigi added a commit to CenterDevice/RestKit that referenced this issue Dec 12, 2013
@blakewatters blakewatters Fix issues with 304 caching when there are no Fetch Request Blocks re…
…gistered that match the URL. closes #1294
361e876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment