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.

Sign up for GitHub

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

Jump to bottom

http: increase default header size from 8KB to 16KB #32520

Closed
wants to merge 4 commits into from
Closed

http: increase default header size from 8KB to 16KB #32520

wants to merge 4 commits into from

Conversation

rosaxxny
Copy link
Contributor

Fixes: #27645

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 27, 2020
@addaleax addaleax added the http Issues or PRs related to the http subsystem. label Mar 27, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 27, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 27, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30144/ (:heavy_check_mark:)

@gengjiawen gengjiawen added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 28, 2020
@addaleax
Copy link
Member

Landed in 400bc5c 🎉

@addaleax addaleax closed this Mar 29, 2020
addaleax pushed a commit that referenced this pull request Mar 29, 2020
@rosaxxny @addaleax
http: increase default header size from 8KB to 16KB
400bc5c 
Fixes: #27645

PR-URL: #32520
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
@rosaxxny @addaleax
http: increase default header size from 8KB to 16KB
bd9f4d2 
Fixes: #27645

PR-URL: #32520
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
Merged
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
4ca9571 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
8130521 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
cf4c332 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
@cekvenich
Copy link

Can we backport this to v12?

@addaleax
Copy link
Member

@cekvenich Yes, I don’t see anything speaking against that. That being said, v12.x supports two separate HTTP parsers, so it might not be trivial to do so.

If you do need a specific default value, I would recommend to always set the CLI option explicitly.

@cekvenich
Copy link

@addaleax Yes, CLI can work, but operational PITA. In v13.x I can also set the size, and I do that. If 12.x had a way to set it or if it had a reasonable default that would be nice. I'm happy 13.x does, so my libs require v13 as min supported. But if 12 had it, than my libs could have 12 as min supported. In any case thank you!!!

@targos targos added backport-blocked-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 22, 2020
@yocontra
Copy link

Just curious about the justification behind this - why go down to 8kb and then back up to 16kb? Is this a happier medium that suits more cases, or is there some other reasoning?

@cekvenich
Copy link

@contra It was 16kb for years, then they dropped it: as this way you can support more connections. But: often just the headers can be large and don't fit in 8K and you get funky messages.. So in v14 they fixed it/reverted it. But 14 is not the 'main' version: 12 is.

@erm1116 erm1116 mentioned this pull request Aug 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@awibox awibox awibox approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase HTTP_MAX_HEADER_SIZE to 16kb