http2: add raw header array support to h2Session.request() by pimterry · Pull Request #57917 · nodejs/node · GitHub

This change allows you to call h2Session.request(['k1', 'v1', 'k2', 'v2']), just like in the HTTP/1 APIs.

Raw header support like this is useful for intermediaries, who generally already have headers in raw format already and would really like to preserve them exactly as received (without any unnecessary reordering or duplicate handling), and for anybody would wants to avoid the effort/perf to objectify headers unnecessarily.

I've added this just to request() for now, but I'd like to add support in the various other HTTP/2 methods - this is a first step to discuss any debatable points here up front.

There's a few notable details in here:

This changes error behaviour in some cases: previously, passing some invalid HTTP header values to request() would destroy the entire underlying session with an error (and would not throw an error)). Not all though - e.g. key header::name would destroy the session with an error, but :unknown-pseudo-header, multiple :method headers or setting HTTP/2-invalid connection/keep-alive/etc headers would throw (all without destroying the session - just failing to create a new stream). AFAICT there is no good reason for this, and it's very awkward & surprising behaviour. I've removed it, and the parts of the tests (test/parallel/test-http2-invalidheaderfield.js) which checked it - this is arguably a breaking change in error handling, let me know what you think.
This does not set stream[kSentHeaders], used by request.sentHeaders. That means that property remains undefined - this matches the behaviour of other APIs like http.request, which doesn't set kOutHeaders for HTTP/1.1 if you pass raw headers. That avoids some work and keeps things consistent, but it's a bit ugly. Opinions welcome - we could easily lazily instantiate sentHeaders instead to avoid any perf hit on the fast path anyway. We could even consider changing HTTP/1.1 to do the same!
There's a fair bit of refactoring here, which notably drops the header name validation at the start of request() - that's because I think the same checks are all run again afterwards within buildNgHeaderString (previously called mapToHeaders) so I'm fairly sure this was adding nothing.

This also notably changes error handling for request(). Previously some invalid header values (but not all) would cause the session to be unnecessarily destroyed automatically, e.g. passing an unparseable header name to request(). This is no longer the case: header validation failures will throw an error, but will not destroy the session or emit 'error' events.

Review requested:

@nodejs/http
@nodejs/http2
@nodejs/net

Codecov Report

❌ Patch coverage is 89.91228% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (33d8e03) to head (f20cf7d).
⚠️ Report is 834 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/util.js	90.44%	17 Missing ⚠️
lib/internal/http2/core.js	88.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57917      +/-   ##
==========================================
- Coverage   90.25%   90.24%   -0.01%     
==========================================
  Files         630      630              
  Lines      185880   186072     +192     
  Branches    36434    36461      +27     
==========================================
+ Hits       167757   167921     +164     
- Misses      11010    11034      +24     
- Partials     7113     7117       +4

Files with missing lines	Coverage Δ
src/node_http_common.h	81.11% <ø> (ø)
lib/internal/http2/core.js	95.34% <88.00%> (-0.22%)	⬇️
lib/internal/http2/util.js	92.35% <90.44%> (-0.65%)	⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lgtm

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

This has a passing CI & one approval, and it's been open for more than 7 days, so it's now mergable! It is a bit fiddly & sensitive, and I want to check for any disagreement before I open more PRs like this for the other header API methods, so I'll leave it a few more days in case anybody else in @nodejs/http2 has opinions - if not, I'll land it sometime next week.

This also notably changes error handling for request(). Previously some invalid header values (but not all) would cause the session to be unnecessarily destroyed automatically, e.g. passing an unparseable header name to request(). This is no longer the case: header validation failures will throw an error, but will not destroy the session or emit 'error' events. PR-URL: #57917 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 4cd8e19

This also notably changes error handling for request(). Previously some invalid header values (but not all) would cause the session to be unnecessarily destroyed automatically, e.g. passing an unparseable header name to request(). This is no longer the case: header validation failures will throw an error, but will not destroy the session or emit 'error' events. PR-URL: #57917 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

When nodejs#57917 added support for sending raw header arrays, Http2Stream#sentHeaders was set only for header objects. This change also sets it for raw headers by lazily instantiating the property to avoid any performance impact on the fast path. Signed-off-by: Darshan Sen <raisinten@gmail.com>

When #57917 added support for sending raw header arrays, Http2Stream#sentHeaders was set only for header objects. This change also sets it for raw headers by lazily instantiating the property to avoid any performance impact on the fast path. Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #59244 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

maybe a bit late - but I guess that should be labeled with semver-minor right?

@Flarna hmm, probably yes. It's not a very significant 'feature' as such, but it is an addition, not a bugfix. I doubt there's much we can usefully do in retrospect there, but I'll keep an eye on that for the subsequent PRs.

Not a big deal I guess. It went in 24.0.0 (major) and 22.17.0 (minor) so fine by accident :o).
Added label now so if it get's backported to 20 proper version increment is ensured.

When #57917 added support for sending raw header arrays, Http2Stream#sentHeaders was set only for header objects. This change also sets it for raw headers by lazily instantiating the property to avoid any performance impact on the fast path. Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #59244 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Apr 17, 2025

mcollina approved these changes Apr 22, 2025

View reviewed changes

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2025

jasnell approved these changes Apr 26, 2025

View reviewed changes

jasnell closed this Apr 26, 2025

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

pimterry mentioned this pull request May 6, 2025

Ensure conflicting flat headers in HTTP/2 are combined correctly nodejs/undici#4196

Merged

7 tasks

github-actions Bot mentioned this pull request Jun 10, 2025

2025-06-24, Version 22.17.0 'Jod' (LTS) #58665

Merged

RaisinTen mentioned this pull request Jul 27, 2025

http2: set Http2Stream#sentHeaders for raw headers #59244

Merged

pimterry mentioned this pull request Aug 12, 2025

http2: add support for raw header arrays in h2Stream.respond() #59455

Merged

Flarna added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 15, 2025

http2: add raw header array support to h2Session.request() by pimterry · Pull Request #57917 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: add raw header array support to h2Session.request() - #57917

http2: add raw header array support to h2Session.request()#57917
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:h2-send-raw-headers

pimterry commented Apr 17, 2025

Uh oh!

nodejs-github-bot commented Apr 17, 2025

Uh oh!

codecov Bot commented Apr 17, 2025 •
edited
Loading

Uh oh!

Uh oh!

mcollina left a comment

Uh oh!

nodejs-github-bot commented Apr 22, 2025

Uh oh!

pimterry commented Apr 25, 2025

Uh oh!

jasnell commented Apr 26, 2025

Uh oh!

Flarna commented Aug 14, 2025

Uh oh!

pimterry commented Aug 14, 2025

Uh oh!

Flarna commented Aug 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

http2: add raw header array support to h2Session.request() by pimterry · Pull Request #57917 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: add raw header array support to h2Session.request() - #57917

http2: add raw header array support to h2Session.request()#57917pimterry wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom pimterry:h2-send-raw-headerspimterry/node:h2-send-raw-headersCopy head branch name to clipboard

Conversation

pimterry commented Apr 17, 2025

Uh oh!

nodejs-github-bot commented Apr 17, 2025

Uh oh!

codecov Bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 22, 2025

Uh oh!

pimterry commented Apr 25, 2025

Uh oh!

jasnell commented Apr 26, 2025

Uh oh!

Flarna commented Aug 14, 2025

Uh oh!

pimterry commented Aug 14, 2025

Uh oh!

Flarna commented Aug 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

http2: add raw header array support to h2Session.request()#57917
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:h2-send-raw-headers

codecov Bot commented Apr 17, 2025 •
edited
Loading