Don't cache `null` responses #27403

devknoll · 2021-07-22T15:41:51Z

It's possible for renderToHTML to return null if res.finished || res.headersSent is true after getInitialProps or getServerSideProps. In such cases, we can't generate a valid ResponseCacheEntry or ResponsePayload, so we shouldn't try.

I took the opportunity to add an invariant for when we expected a cacheable response, but didn't get one. This could happen because Next.js or an application erroneously mutated the underlying res during the rendering of a page with getStaticProps. This shouldn't normally be possible because res isn't exposed in such cases, but it's theoretically possible with a custom server, so it seemed worth flagging.

devknoll · 2021-07-22T16:49:49Z

packages/next/server/next-server.ts

@@ -1641,7 +1646,10 @@ export default class Server {
      } else if (isRedirect) {
        value = { kind: 'REDIRECT', props: pageData }
      } else {
-        value = { kind: 'PAGE', html: html!, pageData }


This is the underlying issue that this PR addresses: It's valid for html to be null in some cases, but not for a cached page. The ! just breaks downstream consumers who expect a valid value here.

shuding

LGTM! Do you want to create a type for ResponseCacheEntry | null?

styfle

Looks good! Just needs an integration test!

ijjk · 2021-07-22T20:42:26Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
buildDuration	13.3s	14.4s	⚠️ +1s
buildDurationCached	3.2s	3.1s	-103ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +2.07 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.252	2.278	⚠️ +0.03
/ avg req/sec	1109.91	1097.5	⚠️ -12.41
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.259	1.288	⚠️ +0.03
/error-in-render avg req/sec	1985.49	1940.54	⚠️ -44.95

Client Bundles (main, webpack, commons)

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
359.HASH.js gzip	2.96 kB	2.96 kB	✓
745.HASH.js gzip	180 B	180 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	21 kB	21 kB	✓
webpack-HASH.js gzip	1.53 kB	1.53 kB	✓
Overall change	67.9 kB	67.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
_app-HASH.js gzip	803 B	803 B	✓
_error-HASH.js gzip	3.06 kB	3.06 kB	✓
amp-HASH.js gzip	554 B	554 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.52 kB	2.52 kB	✓
head-HASH.js gzip	2.28 kB	2.28 kB	✓
hooks-HASH.js gzip	903 B	903 B	✓
image-HASH.js gzip	5.61 kB	5.61 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	319 B	319 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	19.1 kB	19.1 kB	✓

Client Build Manifests

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
_buildManifest.js gzip	490 B	490 B	✓
Overall change	490 B	490 B	✓

Rendered Page Sizes

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
index.html gzip	530 B	530 B	✓
link.html gzip	543 B	543 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
buildDuration	10.9s	10.5s	-457ms
buildDurationCached	4.3s	4.1s	-182ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +2.07 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.46	2.274	-0.19
/ avg req/sec	1016.45	1099.53	+83.08
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.325	1.279	-0.05
/error-in-render avg req/sec	1886.98	1955.01	+68.03

Client Bundles (main, webpack, commons)

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
17.HASH.js gzip	2.98 kB	2.98 kB	✓
18.HASH.js gzip	185 B	185 B	✓
677f882d2ed8..HASH.js gzip	13.7 kB	13.7 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	8.4 kB	8.4 kB	✓
webpack-HASH.js gzip	1.22 kB	1.22 kB	✓
Overall change	68.5 kB	68.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
_app-HASH.js gzip	791 B	791 B	✓
_error-HASH.js gzip	3.76 kB	3.76 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	2.97 kB	2.97 kB	✓
hooks-HASH.js gzip	911 B	911 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.02 kB	3.02 kB	✓
withRouter-HASH.js gzip	294 B	294 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	17.6 kB	17.6 kB	✓

Client Build Manifests

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	azukaru/next.js dont-cache-resolved-responses	Change
index.html gzip	577 B	577 B	✓
link.html gzip	588 B	588 B	✓
withRouter.html gzip	569 B	569 B	✓
Overall change	1.73 kB	1.73 kB	✓

Commit: 7024371

It's possible for `renderToHTML` to return `null` if `res.finished || res.headersSent` is `true` after `getInitialProps` or `getServerSideProps`. In such cases, we can't generate a valid `ResponseCacheEntry` or `ResponsePayload`, so we shouldn't try. I took the opportunity to add an invariant for when we expected a cacheable response, but didn't get one. This could happen because Next.js or an application erroneously mutated the underlying `res` during the rendering of a page with `getStaticProps`. This shouldn't normally be possible because `res` isn't exposed in such cases, but it's theoretically possible with a custom server, so it seemed worth flagging.

Don't cache null responses

06a9995

ijjk added created-by: Chrome Aurora type: next labels Jul 22, 2021

devknoll added 3 commits July 22, 2021 16:19

Add invariant if cache entry is expected

453a338

Remove outdated comment

cbbed91

Merge branch 'canary' into dont-cache-resolved-responses

eb9f032

devknoll marked this pull request as ready for review July 22, 2021 16:43

devknoll requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 22, 2021 16:43

devknoll commented Jul 22, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

shuding approved these changes Jul 22, 2021

View reviewed changes

ijjk approved these changes Jul 22, 2021

View reviewed changes

styfle reviewed Jul 22, 2021

View reviewed changes

Merge branch 'canary' into dont-cache-resolved-responses

a462e5e

This comment has been minimized.

Sign in to view

Merge branch 'canary' into dont-cache-resolved-responses

90e1cac

This comment has been minimized.

Sign in to view

Merge branch 'canary' into dont-cache-resolved-responses

f85e763

This comment has been minimized.

Sign in to view

Merge branch 'canary' into dont-cache-resolved-responses

7024371

kodiakhq bot merged commit 1651129 into vercel:canary Jul 22, 2021

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't cache `null` responses #27403

Don't cache `null` responses #27403

devknoll commented Jul 22, 2021 •

edited

Loading

devknoll Jul 22, 2021 •

edited

Loading

This comment has been minimized.

shuding left a comment

styfle left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 22, 2021

Don't cache null responses #27403

Don't cache null responses #27403

Conversation

devknoll commented Jul 22, 2021 • edited Loading

devknoll Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

shuding left a comment

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 22, 2021

Stats from current PR

Don't cache `null` responses #27403

Don't cache `null` responses #27403

devknoll commented Jul 22, 2021 •

edited

Loading

devknoll Jul 22, 2021 •

edited

Loading