X Tutup
The Wayback Machine - https://web.archive.org/web/20230131200320/https://github.com/nodejs/node/pull/46333
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

http: close the connection after sending a body without declared length #46333

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pimterry
Copy link
Contributor

This is related to #46331 but not directly dependent - it's a separate issue I've just noticed en route.

Previously, if you removed both the content-length and transfer-encoding headers, everything was sent as normal, and the connection would still be kept alive by default. This is problematic, because without those headers to declare the size of the message body, the only way the client knows when the body is completed is when the connection closes. With KA, the connection doesn't close for ages.

See https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 for more details on this message body handling logic. This is the final case (7):

Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

This meant that in effect, if you removed both headers then every response came with a 5 second delay at the end (the default KA timeout) before the client could process it.

With this PR, if you remove both headers then the connection closes automatically immediately, so the client knows straight away that it has received the whole message body and everything works correctly.

This does behave slightly differently with & without #46331:

  • Currently, without that PR, this only affects code that removes the content-length & transfer-encoding headers, but does not remove the connection header (because doing so disables KA, which solves the issue).
    Note that the only test covering this does exactly that, so the modified test here actually still passes on main even without this change.
  • Once that other PR is merged, this issue would affect any code that removes both headers, regardless of whether they remove the Connection header, because doing so no longer disables KA. The modified test here does fail if run against that PR branch (and does pass when run with this test + fix on top of that branch).

Previously, if you removed both content-length and transfer-encoding
headers, the connection would still be kept-alive by default. This isn't
helpful, because without those headers, the only way the client knows
when the body is completed is when the connection closes.

See https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 for more
details on this message body handling logic (this is case 7).

This meant that in effect, if you removed both headers every response
came with a 5 second delay at the end (the default KA timeout) before
the client could process it. Now, if you remove both headers the
connection closes automatically immediately, so the client knows that
it has received the whole message body.
@nodejs-github-bot
Copy link
Contributor

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 24, 2023
ronag
ronag approved these changes Jan 24, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023
@nodejs-github-bot
Copy link
Contributor

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM.

Unfortunately, this is a semver-major change so only Node 20 and beyond will benefit from this.

@ShogunPanda ShogunPanda added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 25, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants
X Tutup