Skip to content
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

Tune fetch for improved connection management #1532

Merged
merged 2 commits into from
Nov 23, 2024
Merged

Tune fetch for improved connection management #1532

merged 2 commits into from
Nov 23, 2024

Conversation

tsightler
Copy link
Collaborator

@tsightler tsightler commented Nov 21, 2024

Hi @dgreif,

This PR is an attempt to address the native fetch/undici issues that were causing http requests to stop. I can provide a hugely detailed analysis of the failure, but, suffice it to say, I believe it has to do with how undici uses keepalive and pool connections. There are quite a number of similar issues on the undici project page.

To work around this, the PR imports Agent from the undici package and creates a custom dispatcher with fewer max TCP connections and much longer keepalive. This makes native NodeJS fetch behave more like browser fetch implementations as well as other http client libs like got (the actual values used are the same as Firefox). This actually has a mostly positive impact for all users as it dramatically reduces the number of DNS lookups and TCP handshakes, since the HTTP connections are now long lived.

So far, I've had 4 users that were experiencing the issue test this patch, and success has been 100%. I've tried to keep the patch as minimal as possible, with only the absolute minimum changes required.

In theory, it should even be possible to do this without pulling in undici dependency as it is possible to modify the global dispatcher, but I had some challenges with that and it felt very hackish. However, if you believe this is better, I can pursue it.

Also, with a custom dispatcher it is possible to enable HTTP/2 via the allowH2: true option. I did test this, however, H2 support in Undici has some pretty ciritcal bugs that regularly caused the NodeJS process to crash (issues are open, hopefully fixed in the next Undici release). I'll monitor this for the future, but it doesn't seem stable for the time being.

@tsightler
Copy link
Collaborator Author

BTW, this is the main blocker for HTTP/2 for now is nodejs/undici#3753. Once it is fixed we can evaluate it's stability again, but we'd need a release of NodeJS that has fixed undici, or we'd have to move to using fetch from undici instead, which is also very easy, just a few minor tweaks to this are needed, if that is preferred.

For now I stayed with native fetch with custom dispatcher instead because my understanding is that there is a plan to expose dispatcher in future Node versions so eventually we can drop undici dependency. Happy to do it either way.

@tsightler tsightler mentioned this pull request Nov 22, 2024
1 task
@dgreif dgreif changed the base branch from beta to main November 23, 2024 00:21
@dgreif dgreif changed the base branch from main to beta November 23, 2024 00:21
Use a custom fetch agent to reduce total TCP connections/DNS lookups and leverage long-lived HTTPS connections
Copy link
Owner

@dgreif dgreif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your all your research and testing on this! I'm very happy we landed on a solution which allows us to use longer keep-alives, rather than terminating every connection. It's unfortunate we have to directly depend on undici, but that's really a quirk of how node is handling this. I'll get this merged and cut a beta release.

I'm also good with keeping this approach, rather than tweaking undici globally. That might impact other things (e.g. plugins) running in the same node instance, which we probably don't want to do 😄

@dgreif dgreif merged commit 209f13e into beta Nov 23, 2024
6 checks passed
@dgreif dgreif deleted the fetch-keepalive branch November 23, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants