Fix cache check race condition #255
Conversation
pcantrell
force-pushed
the
fix-cache-check-race-condition
branch
from
777b21a
to
8e7d99a
Compare
| @@ -513,7 +538,7 @@ public final class Resource: NSObject | |||
|
|
|||
| private func receiveDataNotModified() | |||
There was a problem hiding this comment.
Hello @pcantrell may I ask the real purpose of receiveDataNotModified().
I'm currently using Siesta 1.4.0 and reached this point as possible bug. The same will apply also on the latest 1.4.3 code.
receiveDataNotModified() as I understood is responsible to notify the observers that the resource data is still valid and will not be downloaded from the network. I'm totally ok with it but few line before the latestData?.touch() will actually change the local data and will update then the cache as well using a new timestamp.
The touch() will update the timestamp and the new timestamp will be cached despite no new data comes in from the backend. As a result the local cached data may never expire.
It's a real corner case but I was able to reproduce it just running again the killed application right before the resource expirations time.
As example, if a resource expires in 5 days and the user get back to the application after 4 days, the app may have been killed by the OS because of memory pressure, the cached resource get a new timestamp, invalidating the 5 days expiration planned for the day after. As result loadIfNeeded() will not result in a network call for a total of 9 days, unless the occurrence repeat all over again.
Looks like that lines 543-546 are not necessary at all.
Am I wrong? Maybe I misunderstood the .notChanged use case?
I'm just able to fix it in my code replacing the default EntityCache func updateEntityTimestamp(_ timestamp: TimeInterval, forKey key: CacheKey) implementation with an empty implementation. Probably all of this is not necessary at all.
Thank you in advance
There was a problem hiding this comment.
receiveDataNotModified() is supposed to mean that the server sent a 304, which means “we just checked and yes, this data is still valid.” If the server keeps sending 304s, then yes, the data in the cache should never expire. That’s what lines 543-546 are for.
I believe that you've flagged a real problem, and the mistake is here: my little hack to complete a cache-backed initial request shouldn’t update the timestamp. Would you mind filing an issue for this, so I don’t lose track of it?
There was a problem hiding this comment.
Ok thanks for the explanation. It makes sense. I was confused because receiveDataNotModified() got called only in the pointed case. At least i didn't found any other call in the Siesta codebase.
Probably something still planned to be introduced?
Anyway i will file the bug so you can keep track of it.
Thanks
There was a problem hiding this comment.
The other (original and primary) call is here: https://github.com/bustoutsolutions/siesta/blob/master/Source/Siesta/Resource/Resource.swift#L478
There was a problem hiding this comment.
Oh nice, I missed it because I was searching for receiveDataNotModified()
Thanks for pointing it out.
This fixes #237.