GoogleChromeLabs / sw-toolbox

[Deprecated] A collection of service worker tools for offlining runtime requests
https://developers.google.com/web/tools/workbox/guides/migrations/migrate-from-sw
Apache License 2.0
3.61k stars 331 forks source link

Problem with maxEntries, Caches are deleted after reload page. #265

Closed chenqiushi closed 7 years ago

chenqiushi commented 7 years ago

Hi, everyone

I'm using the maxEntries option to limit cache count. Just like this:

toolbox.options.cache = {
  name: 'TEST-CACHE',
  maxEntries: 5
};
toolbox.router.get(/test\/data\/files\/test-\d*\.js$/, toolbox.networkFirst);

The test page requests 10 javascript resources:

test-1.js, test-2.js, test-3.js ... test-10.js

And yes the cache storage list shows as expected using the LRU strategies. The first 5 are deleted, remaining 5 caches:

test-6.js, test-7.js, test-8.js ... test-10.js

But when I try to reload the page, PROBLEM OCCURS: all the caches in cache storage list are deleted, and the cache list will always be empty even if you reload multiple times. (I check the cache storage list with Chrome devTools )

I expect the cache list should always remain 5 latest resources ?

Does anyone have the same issue ? Or I just misunderstand the LRU strategies with maxExtries ? Could anyone tell me what was wrong. Thanks so much.

easonyq commented 7 years ago

Probably because cache.put and cache.delete is async, so in 'delete phase' all caches have been removed by mistake, like the following case: image

This causes an empty cache storage, which is not expected. @gauntface

jeffposnick commented 7 years ago

Are you seeing this just on the automated test suite, or in a production web application as well?

The test suite page is meant to be run as a one-off starting with a fresh slate, and state might be inconsistent if you reload it.

chenqiushi commented 7 years ago

@jeffposnick , Thanks for your reply. It is not in the automated test suite, I made a simple local page to test. The code looks like: https://jsfiddle.net/pow0tzbo/1/. It's easy to reproduce if you would like to have a try. And I think @easonyq 's console has shown that issue.

If a local demo is created like that, the reproduction step will be:

  1. Load the page make sure service worker has been installed and activated. As expected, the cache storage list has 5 caches(test-6.js to test-10.js) through the Chrome devtools - Application - Cache.

  2. Reload the page, Chrome devtools - Application - Cache - Update caches. All the caches have been removed which is not expected. Reload again and again, occasionally one or two cache will be remained.

I was wondering if the project is still being maintained and is there anyone who had found this issue before. Thanks

mbj36 commented 7 years ago

I am also facing this strange issue! @jeffposnick @chenqiushi Cache is getting deleted on page reload

gauntface commented 7 years ago

A working example (The jsfiddle example doesn't actually work so can't be used for testing): https://plaid-accordion.glitch.me/

Code: https://glitch.com/edit/#!/plaid-accordion?path=public/sw.js:4:15

gauntface commented 7 years ago

I'm really struggling to reproduce this. Are you right clicking on the cache tab and refreshing the caches?

If this is still not working can someone please add a working - complete example in glitch

cache-refresh

chenqiushi commented 7 years ago

@gauntface , In your glitch example, your maxEntries is set to 5 while there are only 3 js files. That is possibly why you could not reproduce.

Please try this example: https://bouncy-waterfall.glitch.me/ code here: https://glitch.com/edit/#!/bouncy-waterfall?path=public/sw.js:12:0

gauntface commented 7 years ago

@chenqiushi thanks for this.

The indexeddb table has 5 entries where as the cache seems to change with each request. My guess is the logic here is completely wonky and you are the first set of people to realise it.

This should be fixed in https://workboxjs.org/

I'm not 100% sure if we'll get a chance to fix this bug in sw-toolbox, but will bring in our team meeting tomorrow.

jeffposnick commented 7 years ago

There does seem to be a race condition:

  1. The list of URLs to delete are read all at once from IDB.
  2. Later, the cache entries corresponding to each URL are deleted one by one.

If there was a fresh cache entry added for a URL in between 1. and 2., then that fresh cache entry will still be deleted in step 2.

Workbox handles those operations differently, and we've got end-to-end tests in place to ensure that burst requests are handled properly.

At this point, years after it was initially implemented, I don't think this is something we should change in sw-toolbox. If you need guarantees about specific cache entries being present, then I'd recommend not using the cache expiration feature, since there's inherently going to be uncertainty about the cache contents.

Moving to Workbox and relying on its improved expiration functionality is a more future-proof option.

gauntface commented 7 years ago

I'm going to close this and we'll add a comment to the project regarding bug fixes soon.