http: correctly calculate strict content length by ronag · Pull Request #46601 · nodejs/node · GitHub

Previously Buffer.byteLength (which is slow) was run regardless of whether or not the len was required.

Review requested:

@nodejs/http
@nodejs/net

lgtm

Failed to start CI

- Validating Jenkins credentials
✖  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/4142770062

I wonder if we could also use the V8 fast calls to improve the underlying byteLengthUtf8() call. That might even improve the situation further, if it's possible?

We already have an ongoing discussion regarding this over at the performance repo.

Here is the link, nodejs/performance#52.

CI: https://ci.nodejs.org/job/node-test-pull-request/49619/

Commit Queue failed

- Loading data for nodejs/node/pull/46601
✔  Done loading data for nodejs/node/pull/46601
----------------------------------- PR info ------------------------------------
Title      http: correctly calculate strict content length (#46601)
Author     Robert Nagy  (@ronag)
Branch     ronag:http-byte-len -> nodejs:main
Labels     http, author ready
Commits    11
 - http: correctly calculate strict content length
 - http: speedup strict content length
 - http: make sure to pass encoding to Buffer.byteLength
 - http: count writtenBytes even if socket is not ready
 - fixup! http: speedup strict content length
 - fixup! http: speedup strict content length
 - fixuP
 - fixuP
 - fixup
 - fixup: linting
 - fixup: cleanup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/46601
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46601
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup: linting
   ⚠  - fixup: cleanup
   ℹ  This PR was created on Fri, 10 Feb 2023 07:55:18 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46601#pullrequestreview-1302089257
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/46601#pullrequestreview-1292827320
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/46601#pullrequestreview-1300561862
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-02-17T10:01:39Z: https://ci.nodejs.org/job/node-test-pull-request/49619/
- Querying data for job/node-test-pull-request/49619/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4234920191

CI: https://ci.nodejs.org/job/node-test-pull-request/49881/

still LGTM

Commit Queue failed

{"message":"Base branch was modified. Review and try the merge again.","documentation_url":"https://docs.github.com/rest/reference/pulls#merge-a-pull-request"}

https://github.com/nodejs/node/actions/runs/4251305015

lgtm

Landed in 9e1824d

Fixes some logical errors related to strict content length. Also, previously Buffer.byteLength (which is slow) was run regardless of whether or not the len was required. PR-URL: #46601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

ronag added the http Issues or PRs related to the http subsystem. label Feb 10, 2023

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 10, 2023

ronag requested review from anonrig and mcollina February 10, 2023 07:55

ronag force-pushed the http-byte-len branch 8 times, most recently from dd09cc5 to 481ed62 Compare February 10, 2023 08:35

mcollina approved these changes Feb 10, 2023

View reviewed changes

ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 10, 2023

github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 10, 2023

ShogunPanda approved these changes Feb 10, 2023

View reviewed changes

ronag mentioned this pull request Feb 10, 2023

http: outputSize and UInt8Array #46605

Closed

debadree25 reviewed Feb 10, 2023

View reviewed changes

Comment thread lib/_http_outgoing.js Outdated

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Feb 12, 2023

ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 15, 2023

ronag force-pushed the http-byte-len branch from 770d6b8 to e88df6e Compare February 17, 2023 09:44

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

fixup: cleanup

20c7f2f

ronag force-pushed the http-byte-len branch from e88df6e to 20c7f2f Compare February 17, 2023 09:50

github-actions Bot mentioned this pull request Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

32 tasks

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 21, 2023

mcollina approved these changes Feb 23, 2023

View reviewed changes

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 23, 2023

mcollina approved these changes Feb 23, 2023

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 23, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2023

nodejs-github-bot merged commit 9e1824d into nodejs:main Feb 23, 2023

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

marco-ippolito mentioned this pull request Jun 8, 2023

http: client inconsistent behavior with content length header #35958

Closed

mhart mentioned this pull request Apr 23, 2025

size is unused in OutgoingMessage.prototype._writeRaw #57985

Open

http: correctly calculate strict content length by ronag · Pull Request #46601 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: correctly calculate strict content length - #46601

http: correctly calculate strict content length#46601nodejs-github-bot merged 11 commits intonodejs:mainnodejs/node:mainfrom nxtedition:http-byte-lennxtedition/node:http-byte-lenCopy head branch name to clipboard

Conversation

ronag commented Feb 10, 2023

Uh oh!

nodejs-github-bot commented Feb 10, 2023 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 10, 2023

Uh oh!

Uh oh!

BridgeAR commented Feb 10, 2023

Uh oh!

ronag commented Feb 10, 2023

Uh oh!

ronag commented Feb 10, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

nodejs-github-bot commented Feb 22, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 23, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

http: correctly calculate strict content length#46601
nodejs-github-bot merged 11 commits into
nodejs:mainfrom
nxtedition:http-byte-len

nodejs-github-bot commented Feb 10, 2023 •
edited by ronag
Loading