Fallback to network on navigationRoute cache miss #1300

jeffposnick · 2018-02-10T22:18:06Z

This is 1/3 of #1188

It adds network fallback behavior to the workbox-routing's navigation route in the default export.

workbox-pr-bot · 2018-02-10T22:30:05Z

PR-Bot Size Plugin

Changed File Sizes

File	Before	After	Change	GZipped
packages/workbox-routing/build/workbox-routing.prod.js	2.77 KB	2.89 KB	+4%	1.32 KB

New Files

No new files have been added.

All File Sizes

View Table

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js	3.27 KB	3.27 KB	0%	1.39 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js	1.07 KB	1.07 KB	0%	573 B
packages/workbox-build/build/_types.js	41 B	41 B	0%	61 B
packages/workbox-build/build/index.js	2.52 KB	2.52 KB	0%	1.06 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js	3.41 KB	3.41 KB	0%	1.26 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js	594 B	594 B	0%	350 B
packages/workbox-cli/build/app.js	5.09 KB	5.09 KB	0%	1.86 KB
packages/workbox-cli/build/bin.js	2.32 KB	2.32 KB	0%	1.03 KB
packages/workbox-core/build/workbox-core.prod.js	6.40 KB	6.40 KB	0%	2.54 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js	2.07 KB	2.07 KB	0%	1.02 KB
packages/workbox-precaching/build/workbox-precaching.prod.js	5.18 KB	5.18 KB	0%	2.02 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js	1.64 KB	1.64 KB	0%	807 B
packages/workbox-routing/build/workbox-routing.prod.js	2.77 KB	2.89 KB	+4%	1.32 KB
packages/workbox-strategies/build/workbox-strategies.prod.js	3.24 KB	3.24 KB	0%	1.01 KB
packages/workbox-sw/build/workbox-sw.js	1.46 KB	1.46 KB	0%	793 B
packages/workbox-webpack-plugin/build/generate-sw.js	6.41 KB	6.41 KB	0%	2.17 KB
packages/workbox-webpack-plugin/build/index.js	742 B	742 B	0%	470 B
packages/workbox-webpack-plugin/build/inject-manifest.js	8.48 KB	8.48 KB	0%	2.77 KB

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

We are using 154% of our max size budget.

Total Size: 22.63KB
Percentage of Size Used: 154%

Gzipped: 9.08KB

gauntface · 2018-02-13T00:37:39Z

packages/workbox-routing/_default.mjs

+      }).catch((error) => {
+        if (process.env.NODE_ENV !== 'production') {
+          logger.debug(`Unable to respond to navigation request with cached ` +
+            `response: ${error}. Falling back to network.`);


This should be error.message or you should log it as a separate log call (possibly in a group).

gauntface

If I set a setCatchHandler what happens? Won't this change block it from working as intended?

Why don't we just have a default catch handler that is just the network given the lengths we are going to basically try and fallback to the network.

jeffposnick · 2018-02-14T20:25:46Z

If I set a setCatchHandler what happens? Won't this change block it from working as intended?

Prior to this PR, any catchHandler you registered will be triggered when there's a cache miss and either a network failure or no response from the network for networkTimeoutSeconds.

With this PR, any catchHandler you registered will be triggered when there's a cache miss and a network failure, independent of networkTimeoutSeconds.

So that's a change in behavior for developers who are already using catchHandlers.

The argument in favor of this PR is that if we're in a world where "unexpected" cache misses are more likely, developers who would not think to use a catchHandler could benefit from Workbox doing more to provide a response by default.

Why don't we just have a default catch handler that is just the network given the lengths we are going to basically try and fallback to the network.

catchHandlers are tied to a Router instance. Making the change to the underlying strategy, like we do in this PR, let's us default to behaviors for certain strategies that isn't tied to using Router.

gauntface · 2018-02-15T19:19:21Z

packages/workbox-routing/_default.mjs

+          logger.debug(`Unable to respond to navigation request with cached ` +
+            `response: ${error.message}. Falling back to network.`);
+        }
+        return fetch(cachedAssetUrl);


Why isn't this the default setCatchHandler? It's in the router module, not the strategy.

gauntface · 2018-02-15T19:20:14Z

@jeffposnick I'm confused by your comment:

Making the change to the underlying strategy, like we do in this PR, let's us default to behaviors for certain strategies that isn't tied to using Router.

This PR is a single change to the Router module.

jeffposnick · 2018-02-15T22:21:37Z

Apologies about that confusion. Serves me right for putting out different PRs in flight at the same time.

gauntface · 2018-02-22T21:01:54Z

@jeffposnick is this still relevant?

jeffposnick · 2018-03-05T16:32:45Z

It's still relevant, but it's not something we need to get implemented in time for v3.

I'd like to revisit some of the "cache resiliency" issues for a minor release, post v3.

jeffposnick · 2018-04-30T01:25:51Z

CC: @philipwalton

So I wanted to revisit this, particularly in light of #1441, in which I believe @coryrylan is bumping up against this edge case. (As far as I can tell this scenario the most likely culprit.)

To clarify some of the crossed wires in response to the initial feedback:

This is a focused changes that detects one of two possible failure modes when the handler associated with workbox.routing.registerNavigationRoute() is executing, and it attempts to read the "fallback ULR" from the cache:

Either there's an exception that occurs during the initial caches.match()
Or there's a cache miss
In either case, this PR will cause the handler to attempt to obtain the fallback URL via the network using fetch(), rather than failing completely (and resulting in a broken navigation).

If that fetch() fails, then any default catchHandler that's registered with workbox.routing will still get a chance to deal with that failure. So this change neither requires or prevents a user from registering a global catchHandler.

Asking a user to manually deal with this situation via their own global catchHandler is problematic, because:

The fallback URL and the request's URL aren't the same, and the catchHandler would only have access to the request's URL. So you wouldn't have the context to know that it's the fallback URL which should be fetch()ed inside of the global catchHandler.
Adding in a default, global catchHandler that always did a fetch() can lead to some pathological scenarios. E.g., a network-first strategy times out after X seconds (and there's nothing in the cache), leading to an exception. If that exception triggers another fetch() automatically, then we're going to end up waiting another X seconds for the second failure before we finally give up. This PR just adds in a single fetch() that would execute if there's an issue with the cached response.

I took another pass through the code and added some comments to clarify these points, but I think it's a safe bet to move ahead with. (And it matches what we were doing in sw-precache previously.)

jeffposnick · 2018-04-30T01:35:00Z

(Also, this PR uses v3 as its base, and it's going to be a mess to rebase to the current master. Assuming there's a 👍, I'm going to close this out and create a fresh PR against master.)

philipwalton · 2018-05-01T16:27:55Z

I'm good with this change, and I think falling back to network anytime we're dealing with cache (with the possible exception of cache only) is probably safest.

jeffposnick · 2018-05-01T16:30:12Z

Thanks, @philipwalton. I'll close this and merge a fresh PR that contains the equivalent change with master as the base.

Fallback to network on navigationRoute cache miss

af6840e

jeffposnick requested review from addyosmani and gauntface February 10, 2018 22:18

gauntface suggested changes Feb 13, 2018

View reviewed changes

Review feedback.

4572737

gauntface suggested changes Feb 14, 2018

View reviewed changes

gauntface reviewed Feb 15, 2018

View reviewed changes

jeffposnick added the Chillin' Not being actively worked on, or deferred for a point in the future. label Mar 5, 2018

jeffposnick mentioned this pull request Apr 20, 2018

Sometimes user can get into a broken state with service worker #1441

Closed

jeffposnick removed the Chillin' Not being actively worked on, or deferred for a point in the future. label Apr 30, 2018

jeffposnick requested review from philipwalton and removed request for addyosmani April 30, 2018 01:26

Added comments.

10585fc

philipwalton approved these changes May 1, 2018

View reviewed changes

jeffposnick closed this May 1, 2018

jeffposnick mentioned this pull request May 1, 2018

Fallback to network if there's a cache miss for registerNavigationRoute #1460

Merged

philipwalton deleted the network-fallback-on-precache-miss branch February 22, 2019 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to network on navigationRoute cache miss #1300

Fallback to network on navigationRoute cache miss #1300

jeffposnick commented Feb 10, 2018

workbox-pr-bot commented Feb 10, 2018

gauntface Feb 13, 2018

gauntface left a comment

jeffposnick commented Feb 14, 2018

gauntface Feb 15, 2018

gauntface commented Feb 15, 2018

jeffposnick commented Feb 15, 2018

gauntface commented Feb 22, 2018

jeffposnick commented Mar 5, 2018

jeffposnick commented Apr 30, 2018

jeffposnick commented Apr 30, 2018

philipwalton commented May 1, 2018

jeffposnick commented May 1, 2018

Fallback to network on navigationRoute cache miss #1300

Fallback to network on navigationRoute cache miss #1300

Conversation

jeffposnick commented Feb 10, 2018

workbox-pr-bot commented Feb 10, 2018

PR-Bot Size Plugin

Changed File Sizes

New Files

All File Sizes

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

gauntface Feb 13, 2018

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

jeffposnick commented Feb 14, 2018

gauntface Feb 15, 2018

Choose a reason for hiding this comment

gauntface commented Feb 15, 2018

jeffposnick commented Feb 15, 2018

gauntface commented Feb 22, 2018

jeffposnick commented Mar 5, 2018

jeffposnick commented Apr 30, 2018

jeffposnick commented Apr 30, 2018

philipwalton commented May 1, 2018

jeffposnick commented May 1, 2018