Opt-in method for purging (some) caches upon a QuotaExceededError #1505
Conversation
|
Thank you for working on this! This is very exciting, as the cache quota reporting chromium bug was recently fixed. With this implementation, one is still possible to manually add logic to retry precaching assets when quota error callbacks are called. |
| @@ -235,7 +242,7 @@ class Plugin { | |||
| * There is no Workbox-specific method needed for cleanup in that case. | |||
| */ | |||
| async deleteCacheAndMetadata() { | |||
| // Do this one at at a time instance of all at once via `Promise.all()` to | |||
| // Do this one at at a time instead of all at once via `Promise.all()` to | |||
There was a problem hiding this comment.
While we're fixing grammar, remove the duplicate "at" earlier in this line.
| sandbox.restore(); | ||
| }); | ||
|
|
||
| devOnly.it(`registerCallback() should throw when passed a non-function`, async function() { |
There was a problem hiding this comment.
Can you wrap all the tests related to registerCallback() in a describe block and then do a separate describe block for all the tests in executeCallback().
There was a problem hiding this comment.
I don't know about that—there's only one test that's just registerCallback() (which only runs in devOnly mode), and all of the rest of them rely on a combination of the two methods. That seems like overkill?
There was a problem hiding this comment.
Sorry if I was unclear. I care less about how many levels of nesting we have than I do about the semantics of describe() and it(). Your test is effectively describing "registerCallback()" and then declaring that it "should throw when passed...", but those semantics are all being passed to the it() function.
While I'm sure it doesn't affect how the test is run, it reads weirdly to me (I read it literally as "it registerCallback should..."). If we're using BDD style blocks, I'd prefer if we stick to the BDD conventions.
| getFriendlyURL, | ||
| logger, | ||
| registerQuotaErrorCallback, |
There was a problem hiding this comment.
What's the reasoning behind exposing this?
There was a problem hiding this comment.
Just flexibility.
While the automatic cleanup of caches that use expiration is one way of dealing with quota errors, you could imagine a web app that, for example, stores a large amount of data in IndexedDB. Developers who understand how their web app stores data could use this to register a callback that cleaned up that hypothetical IndexedDB database.
| @@ -76,6 +79,10 @@ class Plugin { | |||
| this._config = config; | |||
| this._maxAgeSeconds = config.maxAgeSeconds; | |||
| this._cacheExpirations = new Map(); | |||
|
|
|||
| if (config.purgeOnQuotaError) { | |||
| registerCallback(() => this.deleteCacheAndMetadata()); | |||
There was a problem hiding this comment.
Is the plan that registerCallback() might/will get invoked in more places in the future? From what I can tell in this PR, this is the only place it's used, which makes the quota.mjs module seem like over-kill for just this.
There was a problem hiding this comment.
Yes, along the lines of #1505 (comment)
In addition to giving developers a general-purpose way of registering a "cleanup" callback, we may want to make use of it elsewhere in the Workbox codebase—potentially as part of the precaching module, along the lines of #1312.
There was a problem hiding this comment.
Yeah, I get the intention of the change, I'm just wondering if it could be somehow better integrated into the rest of workbox.
Did you consider creating a new lifecycle callback: maybe something like cacheWriteDidFail? And in the same way that we can register a generic fetch failure handler, we could register generic cache failure handler.
Anyway, just a thought, I don't want to block this change if a better solution isn't obvious.
There was a problem hiding this comment.
I felt like using the lifecycle callback naming conventions would be misleading, since here you're registering a callback that has a "global" (or workbox.core) scope, which will be invoked regardless of which strategy triggers the quota exception. That doesn't match the behavior of the lifecycle callbacks, which are all scoped to a given strategy.
workbox.routing.setCatchHandler() is closer in semantics, but that only supports setting a single callback, while we need support for multiple quota exception callbacks. To me, register... sounds more in line with being able to create multiple callbacks than set..., but I could change the naming if you feel strongly.
There was a problem hiding this comment.
I felt like using the lifecycle callback naming conventions would be misleading, since here you're registering a callback that has a "global"...
Right, I wasn't suggesting just using the name, I was suggesting we actually add a cacheWriteDidFail lifecycle callback. While I understand that it's not global and would have to be added per strategy, that seems like a benefit to me, not a shortcoming. And users could write their strategy code in such a way that sharing the same callbacks across all their strategies isn't so bad.
My concern is that global handlers can lead to unpredictable behavior that's harder to test, especially for folks who are using Workbox in a more modular way, or anyone building a third-party plugin.
Anyway, I don't want to block this feature being added, especially if we know folks are running into quota issues today. And you've obviously thought about the trade-offs, so I'm happy with whatever you think is best.
But I think we should revisit it later. I also think it's probably worth adding a catchWriteDidFail lifecycle method at some point regardless, so strategy plugin authors can handle these cases individually without potentially affecting plugins they don't own.
There was a problem hiding this comment.
I filed #1530 to capture that. To address the immediate quota issues, I'd like to go with a global approach.
| callbacks.add(callback); | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| logger.log('Registered a callback to respond to quota errors.', callback); |
There was a problem hiding this comment.
How you checked to see how these function are logged (IIRC normal functions output their toString() result)? Maybe it makes more sense to output .name value?
There was a problem hiding this comment.
Yeah, it logs the code for the actual callback function. This will normally be
() => this.deleteCacheAndMetadata()or it might be the code for a custom callback, if you write and register your own.
I experimented with this a little bit and could go with just leaving out the logging of the function entirely, but I figured that more context for the development environment logging is better.
Since a developer could theoretically pass in whatever function they want here (in addition to using our automatically registered functions), I don't think we could count on .name being meaningful.
There was a problem hiding this comment.
You probably already know this, but .name has gotten a lot better in browsers lately (e.g. it reports anonymous functions assigned to variable or property names). That being said, if the common use case is actually passing an unreferenced anonymous function then yeah, it's probably not useful.
There was a problem hiding this comment.
I'd just like to leave it as-is to make sure we're providing the full context to folks who are in debug mode and reading the Console log messages.
| }); | ||
| plugin.deleteCacheAndMetadata = sandbox.stub(); | ||
|
|
||
| await executeCallbacks(); |
There was a problem hiding this comment.
Same comment here as above w.r.t the name.
There was a problem hiding this comment.
Not sure what you mean—the name of what?
There was a problem hiding this comment.
This is definitely a nit, but I mention it because it confused me a bit when reading the code for the first time.
Basically my point is that the name executeCallbacks() here has no context, so as a reader you have to go searching for where it's defined/imported to figure out what it's doing.
If it were called something like executeQuotaErrorCallbacks() or something like that, then it'd be more clear. Alternatively, if you imported the entire namespace as quotaErrors, then you'd have more context because it'd be quotaErrors.executeCallbacks().
Anyway, like I said this is a nit. If you think it's fine, I'm OK with it.
There was a problem hiding this comment.
Note, this comment primarily applies to line 90 in cacheWrapper, not necessarily here.
There was a problem hiding this comment.
Renamed to executeQuotaErrorCallbacks().
| await Promise.all(requests.map(async (request) => { | ||
| // Process each request/response one at a time, deleting the temporary entry | ||
| // when done, to help avoid triggering quota errors. | ||
| for (const request of requests) { |
There was a problem hiding this comment.
+1 to this change. I think it'll make it a lot easier to respond to errors in a predictable way.
| * a quota error. | ||
| * | ||
| * @param {Function} callback | ||
| * |
There was a problem hiding this comment.
Can you make the spacing here consistent with below. I'm not sure if we prefer a space or not.
|
I've addressed some of the comments, and provided more context. Another thing that I'd like to do is write up a "Guide to Handling Quota Errors" or something along those lines for https://developers.google.com/web/tools/workbox/guides/ following the next release, which will explain a bit more about opting-in to the automatic cleanup and registering your own callback. |
| sandbox.restore(); | ||
| }); | ||
|
|
||
| devOnly.it(`registerCallback() should throw when passed a non-function`, async function() { |
There was a problem hiding this comment.
Sorry if I was unclear. I care less about how many levels of nesting we have than I do about the semantics of describe() and it(). Your test is effectively describing "registerCallback()" and then declaring that it "should throw when passed...", but those semantics are all being passed to the it() function.
While I'm sure it doesn't affect how the test is run, it reads weirdly to me (I read it literally as "it registerCallback should..."). If we're using BDD style blocks, I'd prefer if we stick to the BDD conventions.
| callbacks.add(callback); | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { | ||
| logger.log('Registered a callback to respond to quota errors.', callback); |
There was a problem hiding this comment.
You probably already know this, but .name has gotten a lot better in browsers lately (e.g. it reports anonymous functions assigned to variable or property names). That being said, if the common use case is actually passing an unreferenced anonymous function then yeah, it's probably not useful.
| }); | ||
| plugin.deleteCacheAndMetadata = sandbox.stub(); | ||
|
|
||
| await executeCallbacks(); |
There was a problem hiding this comment.
This is definitely a nit, but I mention it because it confused me a bit when reading the code for the first time.
Basically my point is that the name executeCallbacks() here has no context, so as a reader you have to go searching for where it's defined/imported to figure out what it's doing.
If it were called something like executeQuotaErrorCallbacks() or something like that, then it'd be more clear. Alternatively, if you imported the entire namespace as quotaErrors, then you'd have more context because it'd be quotaErrors.executeCallbacks().
Anyway, like I said this is a nit. If you think it's fine, I'm OK with it.
| }); | ||
| plugin.deleteCacheAndMetadata = sandbox.stub(); | ||
|
|
||
| await executeCallbacks(); |
There was a problem hiding this comment.
Note, this comment primarily applies to line 90 in cacheWrapper, not necessarily here.
| @@ -76,6 +79,10 @@ class Plugin { | |||
| this._config = config; | |||
| this._maxAgeSeconds = config.maxAgeSeconds; | |||
| this._cacheExpirations = new Map(); | |||
|
|
|||
| if (config.purgeOnQuotaError) { | |||
| registerCallback(() => this.deleteCacheAndMetadata()); | |||
There was a problem hiding this comment.
Yeah, I get the intention of the change, I'm just wondering if it could be somehow better integrated into the rest of workbox.
Did you consider creating a new lifecycle callback: maybe something like cacheWriteDidFail? And in the same way that we can register a generic fetch failure handler, we could register generic cache failure handler.
Anyway, just a thought, I don't want to block this change if a better solution isn't obvious.
| import pluginEvents from '../models/pluginEvents.mjs'; | ||
| import pluginUtils from '../utils/pluginUtils.mjs'; | ||
| import {WorkboxError} from './WorkboxError.mjs'; | ||
| import {assert} from './assert.mjs'; | ||
| import {executeCallbacks} from './quota.mjs'; |
There was a problem hiding this comment.
Can we also change this name to executeQuotaErrorCallbacks?
|
Thanks Jeff! I left a few random thoughts for the future and one more comment, but overall everything LGTM. |
PR-Bot Size PluginChanged File Sizes
New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin9.68KB gzip'ed (65% of limit) |
|
When can we expect this to be released? |
R: @philipwalton
CC: @raejin @Enalmada @josephliccini
Fixes #1308
There are two changes in this PR:
There's a check in
workbox-core'scacheWrapper.put()(which is used "under the hood" throughout Workbox) method to detect when the underlyingcache.put()fails due to aQuotaExceededError. When that happens, one or more previously registered callback functions will automatically be called, and given a chance to free up storage. There is (currently) no attempt made to retry the failedcache.put()automatically after the cleanup completes, and theQuotaExceededErrorwill be re-thrown and bubble-up to the method that calledcacheWrapper.put()originally.There's a new
purgeOnQuotaErroroption added to theworkbox.expiration.Pluginconstructor. It defaults tofalse. Whentrue, it will automatically register a quota error callback that will delete the entire cache (and IndexedDB) data that the plugin is responsible for. (This is based on New workbox.expiration.Plugin.deleteCacheAndMetadata() method #1500.)Nothing should change by default, but developers who want to opt-in to trying this automatic deletion of caches should make sure that they're using
workbox.expiration.Pluginfor a given strategy, and setpurgeOnQuotaError: truealongside theirmaxEntriesand/ormaxAgeSecondsconfiguration when constructing the plugin.At some point in the future, we might:
cache.put()operation after all the callbacks are complete.purgeOnQuotaError: trueinside ofworkbox.expiration.Plugin.workbox.precachingto handle quota errors in a different way.In the meantime, I'd love to start getting some feedback from developers who frequently encounter quota issues with their real-world applications, and figure out how effective opting-in to this new behavior is.
It will have the most significant effect on developers whose runtime caches contribute the largest amounts towards their quota usage (perhaps because they're caching opaque responses). If you're precaching a massive amount of data, but not runtime caching much, this PR is not likely to help.