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

fix: Use keep-alive by default for Connection header. #1473

Closed
wants to merge 1 commit into from
Closed

fix: Use keep-alive by default for Connection header. #1473

wants to merge 1 commit into from

Conversation

regseb
Copy link

@regseb regseb commented Jan 23, 2022

Purpose

Use keep-alive by default for Connection header.

Changes

Replace close by keep-alive.

Additional information

Chromium and Firefox use keep-alive by default for Connection header.
MDN specifies close is the default value for HTTP 1.0. And usually, it's keep-alive for HTTP 1.1 (version used by Node.js).


  • I updated ./docs/CHANGELOG.md with a link to this PR or Issue
  • I updated ./docs/v3-UPGRADE-GUIDE
  • I updated readme
  • I added unit test(s)

@regseb regseb mentioned this pull request Mar 5, 2022
31 tasks
@AlttiRi
Copy link

AlttiRi commented Apr 21, 2022

However, will it have any effect on the TCP connection?

I think just setting the Connection header to keep-alive will not have real effect on the connection — it's not enough to just set the header for using the same connection for different fetch calls).

Although, I did not test it.

README.md#default-headers
README.md#custom-agent

dhedey added a commit to dhedey/node-fetch that referenced this pull request Apr 21, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes node-fetch#1735 and likely replaces node-fetch#1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
node-fetch@af21ae6
node-fetch@7f68577
jimmywarting pushed a commit that referenced this pull request Jul 25, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes #1735 and likely replaces #1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
af21ae6
7f68577
dhedey added a commit to dhedey/node-fetch that referenced this pull request Jul 27, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes node-fetch#1735 and likely replaces node-fetch#1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
node-fetch@af21ae6
node-fetch@7f68577

This commit is backported to the v2 branch from node-fetch#1736 against v3.
@dhedey
Copy link
Contributor

dhedey commented Jul 27, 2023

I suggest this should be closed, as it has been obsoleted by #1736 👍

@LinusU LinusU closed this Jul 27, 2023
@regseb regseb deleted the keep-alive branch July 27, 2023 12:49
jimmywarting pushed a commit that referenced this pull request Aug 18, 2023
Instead, we rely on the underlying http implementation in Node.js
to handle this, as per the documentation at
https://nodejs.org/api/http.html#new-agentoptions

This fixes #1735 and likely replaces #1473

The original change introducing this provided no clear motivation
for the override, and the implementation has since been changed to
disable this header when an agent is provided, so I think there
is sufficient evidence that removing this is the correct behaviour.
af21ae6
7f68577

This commit is backported to the v2 branch from #1736 against v3.
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.

4 participants