Call res.write('') inside apiRes.redirect() helper to prevent an edge case #20461

kachkaev · 2020-12-24T20:56:43Z

This PR is a small follow-up to #14705. It saves Next.js users from falling into a pretty nasty trap in which I ended up last Friday. It took more than two days to investigate what was going on, so I hope I'm the last person who’s doing it 😅

Next.js-specific MWE: https://github.com/kachkaev/hanging-response-in-next-via-redirect-plus-compression (needs to be ran locally using Node 14.0.0+).

To reproduce the bug I’m fixing:

Pick a large http body size (64 or 128 KB)
Check Call res.end() after res.redirect() in /api/redirect
Navigate to a heavy page or an api handler via redirect
Observe that the http response is never finished.

If you set compress to false in next.config.js or pick a small payload size (< zlib.Z_DEFAULT_CHUNK after compression), the bug will not be observed. This is explained by the use of res.on("drain", ...) by the compression package. The package itself is not the reason for an issue though, it seems to be in the Node’s built-in http package.

I’m happy to provide more info or GitHub CI to the MWE if needed. I was also thinking of adding some Next.js-specific testing, but could not come up with a compact and clear test plan. Happy to do this if there are any ideas.

cc @botv (author of #14705)

ijjk · 2020-12-24T21:04:47Z

Stats from current PR

Default Server Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	9s	8.5s	-564ms
nodeModulesSize	82.6 MB	82.6 MB	⚠️ +93 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.776	1.598	-0.18
/ avg req/sec	1407.27	1564.29	+157.02
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.141	1.071	-0.07
/error-in-render avg req/sec	2190.46	2335.35	+144.89

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..5e70.js gzip	12.8 kB	12.8 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-e0d2962..b163.js gzip	6.56 kB	6.56 kB	✓
webpack-95c2..e870.js gzip	751 B	751 B	✓
Overall change	59 kB	59 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-d3..23f6.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-0d19cb6..5497.js gzip	1.28 kB	1.28 kB	✓
_error-85785..a9f3.js gzip	3.44 kB	3.44 kB	✓
hooks-42456f..0c06.js gzip	887 B	887 B	✓
index-8081ce..e44f.js gzip	227 B	227 B	✓
link-0ab9f83..fa00.js gzip	1.61 kB	1.61 kB	✓
routerDirect..c3d8.js gzip	303 B	303 B	✓
withRouter-0..a68e.js gzip	302 B	302 B	✓
Overall change	8.05 kB	8.05 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Rendered Page Sizes

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
index.html gzip	612 B	612 B	✓
link.html gzip	620 B	620 B	✓
withRouter.html gzip	607 B	607 B	✓
Overall change	1.84 kB	1.84 kB	✓

Serverless Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	10.8s	10.4s	-468ms
nodeModulesSize	82.6 MB	82.6 MB	⚠️ +93 B

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..5e70.js gzip	12.8 kB	12.8 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-e0d2962..b163.js gzip	6.56 kB	6.56 kB	✓
webpack-95c2..e870.js gzip	751 B	751 B	✓
Overall change	59 kB	59 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-d3..23f6.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-0d19cb6..5497.js gzip	1.28 kB	1.28 kB	✓
_error-85785..a9f3.js gzip	3.44 kB	3.44 kB	✓
hooks-42456f..0c06.js gzip	887 B	887 B	✓
index-8081ce..e44f.js gzip	227 B	227 B	✓
link-0ab9f83..fa00.js gzip	1.61 kB	1.61 kB	✓
routerDirect..c3d8.js gzip	303 B	303 B	✓
withRouter-0..a68e.js gzip	302 B	302 B	✓
Overall change	8.05 kB	8.05 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Serverless bundles Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_error.js	1 MB	1 MB	⚠️ +18 B
404.html	2.67 kB	2.67 kB	✓
hooks.html	1.92 kB	1.92 kB	✓
index.js	1 MB	1 MB	⚠️ +18 B
link.js	1.06 MB	1.06 MB	⚠️ +18 B
routerDirect.js	1.05 MB	1.05 MB	⚠️ +18 B
withRouter.js	1.05 MB	1.05 MB	⚠️ +18 B
Overall change	5.16 MB	5.16 MB	⚠️ +90 B

Commit: 77fc25d

ijjk · 2021-01-03T13:13:08Z

Stats from current PR

Default Server Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	9.9s	10.1s	⚠️ +210ms
nodeModulesSize	80.6 MB	80.6 MB	⚠️ +93 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.998	1.973	-0.02
/ avg req/sec	1251.31	1266.8	+15.49
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.319	1.255	-0.06
/error-in-render avg req/sec	1895.18	1991.98	+96.8

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..42af.js gzip	13 kB	13 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-a9a6f0d..a96d.js gzip	6.59 kB	6.59 kB	✓
webpack-50be..df5b.js gzip	751 B	751 B	✓
Overall change	59.3 kB	59.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-81..14d7.js gzip	31.2 kB	31.2 kB	✓
Overall change	31.2 kB	31.2 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-b6fc6bc..222c.js gzip	1.28 kB	1.28 kB	✓
_error-e2ffa..0f3f.js gzip	3.46 kB	3.46 kB	✓
hooks-010c20..8411.js gzip	887 B	887 B	✓
index-bbee2f..528b.js gzip	227 B	227 B	✓
link-705099c..c35d.js gzip	1.64 kB	1.64 kB	✓
routerDirect..bf84.js gzip	303 B	303 B	✓
withRouter-a..5826.js gzip	302 B	302 B	✓
Overall change	8.09 kB	8.09 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Rendered Page Sizes

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
index.html gzip	615 B	615 B	✓
link.html gzip	621 B	621 B	✓
withRouter.html gzip	607 B	607 B	✓
Overall change	1.84 kB	1.84 kB	✓

Serverless Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	12s	12.2s	⚠️ +238ms
nodeModulesSize	80.6 MB	80.6 MB	⚠️ +93 B

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..42af.js gzip	13 kB	13 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-a9a6f0d..a96d.js gzip	6.59 kB	6.59 kB	✓
webpack-50be..df5b.js gzip	751 B	751 B	✓
Overall change	59.3 kB	59.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-81..14d7.js gzip	31.2 kB	31.2 kB	✓
Overall change	31.2 kB	31.2 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-b6fc6bc..222c.js gzip	1.28 kB	1.28 kB	✓
_error-e2ffa..0f3f.js gzip	3.46 kB	3.46 kB	✓
hooks-010c20..8411.js gzip	887 B	887 B	✓
index-bbee2f..528b.js gzip	227 B	227 B	✓
link-705099c..c35d.js gzip	1.64 kB	1.64 kB	✓
routerDirect..bf84.js gzip	303 B	303 B	✓
withRouter-a..5826.js gzip	302 B	302 B	✓
Overall change	8.09 kB	8.09 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Serverless bundles Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_error.js	1 MB	1 MB	⚠️ +18 B
404.html	2.67 kB	2.67 kB	✓
hooks.html	1.92 kB	1.92 kB	✓
index.js	1 MB	1 MB	⚠️ +18 B
link.js	1.06 MB	1.06 MB	⚠️ +18 B
routerDirect.js	1.05 MB	1.05 MB	⚠️ +18 B
withRouter.js	1.05 MB	1.05 MB	⚠️ +18 B
Overall change	5.17 MB	5.17 MB	⚠️ +90 B

Commit: b834f39

timneutkens

Can you add an integration test for this?

kachkaev · 2021-01-03T22:59:41Z

Can you add an integration test for this?

I was thinking of adding a test, but could not come up with a good test plan that would not be too disruptive for the existing CI setup. The upstream bug is going to be fixed in Node.js itself, so for us to have a breakable test, we need to stick with Node 14.15.3 or whatever last v14 / v15 out there before the permanent fix is available.

The already existing tests demonstrate that I have not broken anything, which is probably enough for this quite peculiar case. That said, I’m happy to add tests if someone could help me a bit with ideas.

timneutkens · 2021-01-04T09:53:09Z

Sounds fine for now then 👍

ijjk · 2021-01-04T10:01:37Z

Stats from current PR

Default Server Mode (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	9.3s	9.5s	⚠️ +225ms
nodeModulesSize	80.6 MB	80.6 MB	⚠️ +93 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.826	1.913	⚠️ +0.09
/ avg req/sec	1368.97	1306.9	⚠️ -62.07
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.164	1.184	⚠️ +0.02
/error-in-render avg req/sec	2146.87	2111.65	⚠️ -35.22

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..396f.js gzip	13 kB	13 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-a9a6f0d..a96d.js gzip	6.59 kB	6.59 kB	✓
webpack-50be..df5b.js gzip	751 B	751 B	✓
Overall change	59.3 kB	59.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-81..14d7.js gzip	31.2 kB	31.2 kB	✓
Overall change	31.2 kB	31.2 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-b6fc6bc..222c.js gzip	1.28 kB	1.28 kB	✓
_error-e2ffa..0f3f.js gzip	3.46 kB	3.46 kB	✓
hooks-010c20..8411.js gzip	887 B	887 B	✓
index-bbee2f..528b.js gzip	227 B	227 B	✓
link-705099c..c35d.js gzip	1.64 kB	1.64 kB	✓
routerDirect..bf84.js gzip	303 B	303 B	✓
withRouter-a..5826.js gzip	302 B	302 B	✓
Overall change	8.09 kB	8.09 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Rendered Page Sizes

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
index.html gzip	615 B	615 B	✓
link.html gzip	621 B	621 B	✓
withRouter.html gzip	607 B	607 B	✓
Overall change	1.84 kB	1.84 kB	✓

Serverless Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
buildDuration	11.1s	11.3s	⚠️ +139ms
nodeModulesSize	80.6 MB	80.6 MB	⚠️ +93 B

Client Bundles (main, webpack, commons)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
677f882d2ed8..396f.js gzip	13 kB	13 kB	✓
framework.HASH.js gzip	39 kB	39 kB	✓
main-a9a6f0d..a96d.js gzip	6.59 kB	6.59 kB	✓
webpack-50be..df5b.js gzip	751 B	751 B	✓
Overall change	59.3 kB	59.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
polyfills-81..14d7.js gzip	31.2 kB	31.2 kB	✓
Overall change	31.2 kB	31.2 kB	✓

Client Pages

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_app-b6fc6bc..222c.js gzip	1.28 kB	1.28 kB	✓
_error-e2ffa..0f3f.js gzip	3.46 kB	3.46 kB	✓
hooks-010c20..8411.js gzip	887 B	887 B	✓
index-bbee2f..528b.js gzip	227 B	227 B	✓
link-705099c..c35d.js gzip	1.64 kB	1.64 kB	✓
routerDirect..bf84.js gzip	303 B	303 B	✓
withRouter-a..5826.js gzip	302 B	302 B	✓
Overall change	8.09 kB	8.09 kB	✓

Client Build Manifests

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_buildManifest.js gzip	323 B	323 B	✓
Overall change	323 B	323 B	✓

Serverless bundles Overall increase ⚠️

	vercel/next.js canary	kachkaev/next.js prevent-edge-case-in-redirect	Change
_error.js	1 MB	1 MB	⚠️ +18 B
404.html	2.67 kB	2.67 kB	✓
hooks.html	1.92 kB	1.92 kB	✓
index.js	1 MB	1 MB	⚠️ +18 B
link.js	1.06 MB	1.06 MB	⚠️ +18 B
routerDirect.js	1.05 MB	1.05 MB	⚠️ +18 B
withRouter.js	1.05 MB	1.05 MB	⚠️ +18 B
Overall change	5.17 MB	5.17 MB	⚠️ +90 B

Commit: 33f6bff

Call res.write('') inside apiRes.redirect() helper

77fc25d

kachkaev requested review from ijjk, lfades, Timer and timneutkens as code owners December 24, 2020 20:56

ijjk added the type: next label Dec 24, 2020

vercel bot temporarily deployed to Preview December 24, 2020 20:56 Inactive

Merge branch 'canary' into prevent-edge-case-in-redirect

b834f39

timneutkens requested changes Jan 3, 2021

View reviewed changes

timneutkens approved these changes Jan 4, 2021

View reviewed changes

Merge branch 'canary' into prevent-edge-case-in-redirect

33f6bff

kodiakhq bot merged commit 7076758 into vercel:canary Jan 4, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call res.write('') inside apiRes.redirect() helper to prevent an edge case #20461

Call res.write('') inside apiRes.redirect() helper to prevent an edge case #20461

kachkaev commented Dec 24, 2020 •

edited

Loading

ijjk commented Dec 24, 2020

ijjk commented Jan 3, 2021

timneutkens left a comment

kachkaev commented Jan 3, 2021 •

edited

Loading

timneutkens commented Jan 4, 2021

ijjk commented Jan 4, 2021

Call res.write('') inside apiRes.redirect() helper to prevent an edge case #20461

Call res.write('') inside apiRes.redirect() helper to prevent an edge case #20461

Conversation

kachkaev commented Dec 24, 2020 • edited Loading

ijjk commented Dec 24, 2020

Stats from current PR

ijjk commented Jan 3, 2021

Stats from current PR

timneutkens left a comment

Choose a reason for hiding this comment

kachkaev commented Jan 3, 2021 • edited Loading

timneutkens commented Jan 4, 2021

ijjk commented Jan 4, 2021

Stats from current PR

kachkaev commented Dec 24, 2020 •

edited

Loading

kachkaev commented Jan 3, 2021 •

edited

Loading