Add `keepAlive` to `node-fetch` polyfill #27376

styfle · 2021-07-21T17:28:57Z

This PR adds a default agent as described in the node-fetch docs.

We should see about 2x perf according to some benchmarks.

ijjk · 2021-07-22T03:03:19Z

packages/next/server/node-polyfill-fetch.js


 // Polyfill fetch() in the Node.js environment
 if (!global.fetch) {
-  global.fetch = fetch
+  const httpAgent = new HttpAgent({ keepAlive: true })
+  const httpsAgent = new HttpsAgent({ keepAlive: true })


Should we implement some other keepAlive optimizations like the ones done in this module? https://www.npmjs.com/package/agentkeepalive for example the socket.setNoDelay optimization seems interesting

There are a lot of knobs to turn here.

The default maxSockets and maxTotalSockets are set to Infinity. So configuring these wouldn't really increase performance but might improve stability.

Using socket.setNoDelay() seems like it could improve performance in some cases, but the default delayed behavior (Nagles Algorithm) already "optimizes throughput at the expense of latency".

Finally, theres scheduling that was actually changed in Node.js 15.6.0 from FIFO to LIFO in nodejs/node#36685

ijjk

Changes look good, thanks!

ijjk · 2021-07-22T14:34:37Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
buildDuration	13s	12.6s	-317ms
buildDurationCached	2.9s	3.1s	⚠️ +211ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +1.56 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.092	1.998	-0.09
/ avg req/sec	1195.2	1251.49	+56.29
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.232	1.145	-0.09
/error-in-render avg req/sec	2029.28	2183.61	+154.33

Client Bundles (main, webpack, commons)

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
_buildManifest.js gzip	490 B	490 B	✓
Overall change	490 B	490 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	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 (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
buildDuration	10.2s	10.2s	⚠️ +5ms
buildDurationCached	3.8s	3.8s	⚠️ +4ms
nodeModulesSize	49.5 MB	49.5 MB	⚠️ +1.56 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.039	2.134	⚠️ +0.09
/ avg req/sec	1226.13	1171.65	⚠️ -54.48
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.153	1.31	⚠️ +0.16
/error-in-render avg req/sec	2168.94	1909.06	⚠️ -259.88

Client Bundles (main, webpack, commons)

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	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	vercel/next.js add-keepalive-to-node-fetch-polyfill	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js add-keepalive-to-node-fetch-polyfill	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: 98f1f7a

gu-stav · 2021-07-23T05:50:13Z

@styfle I've deployed the latest canary and now I'm seeing errors during the build:

Jul 23 07:45:29 AM    error: [CombinedError: [Network] request to http://.../graphql failed, reason: socket hang up] {
Jul 23 07:45:29 AM      graphQLErrors: [],
Jul 23 07:45:29 AM      networkError: FetchError: request to http://..../graphql failed, reason: socket hang up
at ClientRequest.<anonymous> (/opt/render/project/src/node_modules/node-fetch/lib/index.js:1461:11)
at ClientRequest.emit (events.js:376:20)
at Socket.socketOnEnd (_http_client.js:499:9)
at Socket.emit (events.js:388:22)
at endReadableNT (internal/streams/readable.js:1336:12)
at processTicksAndRejections (internal/process/task_queues.js:82:21)

I'm using urql and have

experimental: {
  staticPageGenerationTimeout: 60 * 3
}

set, because some requests take quite some time. Could this be related to this PR?

styfle · 2021-07-23T17:04:22Z

@gu-stav You could try running this code prior to any HTTP requests to disable keep-alive and use the default agent

const { Agent } = require('http')
const agent = new Agent()
global.fetch = (url, opts) => fetch(url, { agent })

// urql or fetch here

If that works then it is likely that this PR caused the issue.

gu-stav · 2021-07-27T19:15:23Z

@styfle sorry, I had to work on something else the last couple of days. I've tried your solution and changed it a little, because it caused recursion:

import { Agent } from 'http';
import fetch from 'node-fetch';

const agent = new Agent();

global.fetch = (url, opts, ...rest) => fetch(url, { ...opts, agent }, ...rest);

and it indeed solves the issue I've mentioned. Should I create a new issue for that, or would you say this is expected behavior and maybe needs documentation?

styfle · 2021-07-27T19:48:04Z

I see. So it sounds like the server doesn't support HTTP keep-alive?

Unless dealing with a very old system, which doesn't support a persistent connection, there is no compelling reason to use this (Short-lived connections) model.

I think we'll need to add a toggle to turn it off in next.config.js 🤔

But it would be great if you could share which URL is causing the issue.

gu-stav · 2021-07-27T19:53:40Z

@styfle The following GraphQL endpoint causes the issue: http://37.228.129.114:1338/graphql It is served by strapi ("koa": "^2.13.1",).

Edit: There is a second strapi instance which is running at https://cms.seebruecke.org/graphql . This one is behind nginx and seems not to have the issue ... 🤔

styfle · 2021-07-27T20:00:28Z

That's odd, the first server is advertising keep-alive so I would expect it to work. Perhaps its misconfigured?

$ curl -i http://37.228.129.114:1338/graphql
HTTP/1.1 400 Bad Request
X-Powered-By: Strapi <strapi.io>
Connection: keep-alive
Keep-Alive: timeout=5

styfle · 2021-08-02T21:45:08Z

We'll have an option to override the new defaults in PR #27709

Follow up to #27376 so users can disable keep-alive. See comment #27376 (comment)

Fixes vercel#27109 This PR adds a default `agent` as described in the [`node-fetch` docs](https://github.com/node-fetch/node-fetch#custom-agent). We should see about 2x perf according to some [benchmarks](https://github.com/Ethan-Arrowood/undici-fetch/blob/main/benchmarks.md#fetch).

…7709) Follow up to vercel#27376 so users can disable keep-alive. See comment vercel#27376 (comment)

Add keepAlive to node-fetch polyfill

e9502c0

ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Jul 21, 2021

This comment has been minimized.

Sign in to view

styfle added 2 commits July 21, 2021 13:43

Fix lint

ac4cee3

Add test

2704384

This comment has been minimized.

Sign in to view

styfle added 2 commits July 21, 2021 18:09

Fix tests

bdd8968

Merge branch 'canary' into add-keepalive-to-node-fetch-polyfill

b91312a

styfle marked this pull request as ready for review July 21, 2021 22:12

styfle requested review from huozhi, ijjk, padmaia, shuding and timneutkens as code owners July 21, 2021 22:12

This comment has been minimized.

Sign in to view

Fix lint

021bc40

This comment has been minimized.

Sign in to view

ijjk reviewed Jul 22, 2021

View reviewed changes

ijjk approved these changes Jul 22, 2021

View reviewed changes

Merge branch 'canary' into add-keepalive-to-node-fetch-polyfill

98f1f7a

kodiakhq bot merged commit 79ed8e1 into canary Jul 22, 2021

kodiakhq bot deleted the add-keepalive-to-node-fetch-polyfill branch July 22, 2021 14:34

gu-stav pushed a commit to seebruecke-org/frontend that referenced this pull request Jul 27, 2021

API: workaround for vercel/next.js#27376

f512395

styfle mentioned this pull request Aug 2, 2021

Add next.config.js option to override default keepAlive #27709

Merged

kodiakhq bot pushed a commit that referenced this pull request Aug 3, 2021

Add next.config.js option to override default keepAlive (#27709)

46e7658

Follow up to #27376 so users can disable keep-alive. See comment #27376 (comment)

flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021

Add next.config.js option to override default keepAlive (vercel#2…

f6a2ac7

…7709) Follow up to vercel#27376 so users can disable keep-alive. See comment vercel#27376 (comment)

matt-dalton mentioned this pull request Nov 10, 2021

E-connection reset error on getInitialProps when requestion from server. #9006

Closed

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

Add `keepAlive` to `node-fetch` polyfill #27376

Add `keepAlive` to `node-fetch` polyfill #27376

styfle commented Jul 21, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk Jul 22, 2021

styfle Jul 22, 2021

ijjk left a comment

ijjk commented Jul 22, 2021

gu-stav commented Jul 23, 2021 •

edited

Loading

styfle commented Jul 23, 2021 •

edited

Loading

gu-stav commented Jul 27, 2021 •

edited

Loading

styfle commented Jul 27, 2021

gu-stav commented Jul 27, 2021 •

edited

Loading

styfle commented Jul 27, 2021 •

edited

Loading

styfle commented Aug 2, 2021

Add keepAlive to node-fetch polyfill #27376

Add keepAlive to node-fetch polyfill #27376

Conversation

styfle commented Jul 21, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk Jul 22, 2021

Choose a reason for hiding this comment

styfle Jul 22, 2021

Choose a reason for hiding this comment

ijjk left a comment

Choose a reason for hiding this comment

ijjk commented Jul 22, 2021

Stats from current PR

gu-stav commented Jul 23, 2021 • edited Loading

styfle commented Jul 23, 2021 • edited Loading

gu-stav commented Jul 27, 2021 • edited Loading

styfle commented Jul 27, 2021

gu-stav commented Jul 27, 2021 • edited Loading

styfle commented Jul 27, 2021 • edited Loading

styfle commented Aug 2, 2021

Add `keepAlive` to `node-fetch` polyfill #27376

Add `keepAlive` to `node-fetch` polyfill #27376

styfle commented Jul 21, 2021 •

edited

Loading

gu-stav commented Jul 23, 2021 •

edited

Loading

styfle commented Jul 23, 2021 •

edited

Loading

gu-stav commented Jul 27, 2021 •

edited

Loading

gu-stav commented Jul 27, 2021 •

edited

Loading

styfle commented Jul 27, 2021 •

edited

Loading