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
Precaching temp #1342
Precaching temp #1342
Changes from 3 commits
f92bcab
e91305e
36664fa
297b10a
99ae898
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,11 +164,15 @@ class PrecacheController { | |
} | ||
} | ||
|
||
// Clear any existing temp cache | ||
await caches.delete(this._getTempCacheName()); | ||
|
||
const entriesToPrecache = []; | ||
const entriesAlreadyPrecached = []; | ||
|
||
for (const precacheEntry of this._entriesToCacheMap.values()) { | ||
if (await this._precacheDetailsModel._isEntryCached(precacheEntry)) { | ||
if (await this._precacheDetailsModel._isEntryCached( | ||
this._cacheName, precacheEntry)) { | ||
entriesAlreadyPrecached.push(precacheEntry); | ||
} else { | ||
entriesToPrecache.push(precacheEntry); | ||
|
@@ -177,7 +181,7 @@ class PrecacheController { | |
|
||
// Wait for all requests to be cached. | ||
await Promise.all(entriesToPrecache.map((precacheEntry) => { | ||
return this._cacheEntry(precacheEntry, options.plugins); | ||
return this._cacheEntryInTemp(precacheEntry, options.plugins); | ||
})); | ||
|
||
if (process.env.NODE_ENV !== 'production') { | ||
|
@@ -190,6 +194,40 @@ class PrecacheController { | |
}; | ||
} | ||
|
||
/** | ||
* Takes the current set of temporary files and moves them to the final | ||
* cache, deleting the temporary cache once copying is complete. | ||
* | ||
* @return { | ||
* Promise<workbox.precaching.CleanupResult>} | ||
* Resolves with an object containing details of the deleted cache requests | ||
* and precache revision details. | ||
*/ | ||
async activate() { | ||
const tempCache = await caches.open(this._getTempCacheName()); | ||
|
||
const requests = await tempCache.keys(); | ||
await Promise.all(requests.map(async (request) => { | ||
const response = await tempCache.match(request); | ||
await cacheWrapper.put(this._cacheName, request, response); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have #1312 open to track the larger question of how to deal with quota issues that come up during precaching. In the meantime, this PR introduces some overhead due to every response in the temp cache being duplicated first, and with all the temp entries only getting deleted after all of those duplicates are written. What if you added in await cacheWrapper.delete(this._getTempCacheName(), request); inside this loop, to clear out the temporary copy of each individual response after it's been duplicated to the final cache? You can still leave the await caches.delete(this._getTempCacheName()); outside the loop, to delete the cache itself, though at that point the cache should have 0 entries in it. |
||
})); | ||
|
||
await caches.delete(this._getTempCacheName()); | ||
|
||
return this._cleanup(); | ||
} | ||
|
||
/** | ||
* Returns the name of the temporary cache. | ||
* | ||
* @return {string} | ||
* | ||
* @private | ||
*/ | ||
_getTempCacheName() { | ||
return `${this._cacheName}-temp`; | ||
} | ||
|
||
/** | ||
* Requests the entry and saves it to the cache if the response | ||
* is valid. | ||
|
@@ -203,7 +241,7 @@ class PrecacheController { | |
* promise resolves with true if the entry was cached / updated and | ||
* false if the entry is already cached and up-to-date. | ||
*/ | ||
async _cacheEntry(precacheEntry, plugins) { | ||
async _cacheEntryInTemp(precacheEntry, plugins) { | ||
let response = await fetchWrapper.fetch( | ||
precacheEntry._networkRequest, | ||
null, | ||
|
@@ -214,7 +252,7 @@ class PrecacheController { | |
response = await cleanRedirect(response); | ||
} | ||
|
||
await cacheWrapper.put(this._cacheName, | ||
await cacheWrapper.put(this._getTempCacheName(), | ||
precacheEntry._cacheRequest, response, plugins); | ||
|
||
await this._precacheDetailsModel._addEntry(precacheEntry); | ||
|
@@ -232,8 +270,10 @@ class PrecacheController { | |
* Promise<workbox.precaching.CleanupResult>} | ||
* Resolves with an object containing details of the deleted cache requests | ||
* and precache revision details. | ||
* | ||
* @private | ||
*/ | ||
async cleanup() { | ||
async _cleanup() { | ||
const expectedCacheUrls = []; | ||
this._entriesToCacheMap.forEach((entry) => { | ||
const fullUrl = new URL(entry._cacheRequest.url, location).toString(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The background on why we were waiting for the correct SW to take control, rather than waiting on activation, is in w3c/ServiceWorker#799 (comment)
We've seen flakiness in unit tests in the past due to the small window of time in between when a service worker activates and when it takes control of a window client. If the window client makes a network request in that window of time, the correct service worker won't intercept it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I saw flakiness the other way, so how about we check for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we dig into the flakiness you saw in the other direction? Potentially outside of this PR, if you can restore the previous logic and supplement it with the additional logic that's now necessarily.
FWIW, a scenario in which a SW can take control of a page before it's been activated sounds like a (potentially serious) browser bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke to @jakearchibald about this as I don't think it is a bug.
Having multiple evens, one claiming and others doing work means:
This makes sense to me but does bring in to question whether we should do some other magic around clientsClaim to account for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, really? My understanding was that the service worker couldn't take control of a client until after it activated. Huh.