Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of quota exceeded errors in workbox-strategies #1308

Closed
jeffposnick opened this issue Feb 13, 2018 · 8 comments
Closed

Handling of quota exceeded errors in workbox-strategies #1308

jeffposnick opened this issue Feb 13, 2018 · 8 comments

Comments

@jeffposnick
Copy link
Contributor

Library Affected:
workbox-strategies

There's be an uptick of issues related to DOMException: Quota exceeded. reported recently, both from Workbox users and from the larger development community. A few bugs in Chrome, along with what appears to be a stricter enforcement of per-origin quota limitations (especially when opaque responses are cached) have all brought this to the forefront.

This is likely to remain an issue if Safari goes ahead with a maximum limit of 50 MiB of cache storage when they ship service workers, as they've indicated.

We could do more to ensure that, when appropriate, Workbox will catch DOMException: Quota exceeded. that occur when writing to caches, and ensure that they lead to failures in passing along a valid response to the client page.

I think it makes sense to address this at the workbox-strategies level, rather than in the underlying cacheWrapper from workbox-core, because we want the flexibility to treat a cache.put() as a fatal exception for some use cases. A failure to write to caches within workbox-precaching should still be fatal, and cause the install event to fail, for instance.

I'm thinking changing code like

event.waitUntil(
cacheWrapper.put(
this._cacheName,
event.request,
responseClone,
this._plugins
)
);

to be:

event.waitUntil(
  cacheWrapper.put(
    this._cacheName,
    event.request,
    responseClone,
    this._plugins
  ).catch((error) => {
    // Log the error, but don't reject.
  });
);

would help, along with corresponding changes in the other workbox-strategies.

What do you think, @gauntface & co.?

@jeffposnick
Copy link
Contributor Author

jeffposnick commented Feb 13, 2018

I'm going to add that based on my reading of the code flow, I'm not sure whether our current implementation would necessarily lead to failures to get a valid response to the page.

In the quota exceeded scenario, we'd still end up passing a valid response to event.respondWith(), but also pass in a promise to event.waitUntil() which rejects, and I'm not clear what the expected outcome would be in that situation. (Perhaps it's a race condition?)

But even if making this change is not strictly necessary, it would be cleaner to put our own logging in via cacheWrapper.put().catch() than it is to rely on whatever error the browser would end up logging by default.

@gauntface
Copy link

@jeffposnick couple of points you've raised:

  1. Custom error logging vs browser. I'm happy to do this if we know where the error has occured (i.e. oh the put failed) but we really need to ensure the browser error message is logged untouched. Covering that error up can lead to bad situations where users can't detect and fix the underlying issue and we can't help debug Workbox with reports where we've covered up the issue.
  2. event.waitUntil() should be ok to accept a rejecting promise - it's intended use is to keep the service worker alive, so I'm not sure why we wouldn't want it to reject.

Could you explain what is happening today for users of Workbox and what you want the actual experience to be.

@gauntface
Copy link

@jeffposnick If we can reproduce the bad behavior (i.e. runtime cache HTML pages, load page in incognito and cache a large number of opaque requests - breaking the 120MB quota and then ensuring the page still loads).

@jeffposnick
Copy link
Contributor Author

@gauntface, thanks for clarifying what the expected behavior of passing a rejected promise to event.waitUntil() is. It sounds like we don't need to add in a .catch() there in order to ensure that a response makes it back to the page, so forget that part.

But, approaching it from a different direction, let's say that a developer ends up in a situation where they've got runtime caching set up and they eventually bump against the quota limit. What, if anything, could Workbox do to help recover from that state automatically?

If the runtime strategy is configured with cache expiration and maxAgeSeconds, a developer might assume that the older entries will eventually be deleted. But the code that triggers the cache expiration logic in v3 runs after await cache.put(). If writing to the cache fails due to a quota exceeded error, then I don't believe that we'll ever get a chance to run the cache expiration code:

// Regardless of whether or not we'll end up invoking
// cacheDidUpdate, wait until the cache is updated.
await cache.put(request, responseToCache);
for (let plugin of updatePlugins) {
await plugin[pluginEvents.CACHE_DID_UPDATE].call(plugin, {
cacheName,
request,
oldResponse,
newResponse: responseToCache,
});
}

If we were to look for a solution outside of the current model, I could imagine that anytime a cache.put() fails for a runtime strategy, we detect whether the failure was due to a quota exceeded error. If it is, then we use our knowledge of which caches are configured with workbox-cache-expiration and forcibly delete all entries in those caches, in the hopes of freeing up space. My assumption is that if a developer explicitly opts-in to a cache expiration policy, they should already be able to deal with arbitrary deletions.

@gauntface
Copy link

Why don't we add an plugin error callback. call the functions and throw and error?

@gauntface
Copy link

Also just thinking more holistically - quota is going to be a pretty bad situation and doing it on a per route basis feels error prone. Should we have a custom callback that gets called when quota is reached and we clear out all caches we are aware of (apart from pre-caching)?

@Enalmada
Copy link

I am trying to deal with this "Uncaught (in promise) DOMException: Quota exceeded." message happening in dev/prod after normal site use (non-incognito):
image

I updated to workbox v3, made sure all the assets I am caching are non-opaque, added maxEntries/Age to everything. For background I basically only runtime cache...cacheFirst versioned css/js/images and networkFirst everything else to obtain minor offline site ability. (BTW, I don't feel like the things I care about would add up to be even close to storage limits...it might be nice for debug mode to output what it thinks are all the current caches and their total size to help people debug this issue.)

I think a sensible default for would be to completely clear out the cache and try again. I think this eliminates the pain the average user is experiencing in production right now. Then in subsequent releases you could roll out features for edge cases...global or per strategy ways of overriding what happens when limits are hit.

@raejin
Copy link

raejin commented Apr 4, 2018

@gauntface @jeffposnick

For the initial installation, if there isn't enough storage for precaching assets, then service worker installation will fail. Which is fine, since it is not actionable if the initial allocated storage is smaller.

If an update is found for a newer version of service worker script, but this time around it fails again not because of the storage allocation problem but the aggregated runtime caches are taking up the space. What do you think about deleting runtime caches in favor of freeing up space for precached assets?


Quoted from @jeffposnick

If it is, then we use our knowledge of which caches are configured with workbox-cache-expiration and forcibly delete all entries in those caches, in the hopes of freeing up space.

This sounds like a reasonable strategy for runtime caches that are configured with expiration strategy. Do you think it'd be useful to have a default strategy in place when quota limit is hit? For example, we can start with the oldest cache, or the same file with different fingerprint etc..

Any more followup from this topic? I have been running into this issue as well, and have been contemplating on a good strategy for quota limit 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants