fix(fetch): send headers in the case that they were sent

[KhafraDev]

This fixes an outstanding issue in fetch where header names were treated as case insensitive, always being sent to the server as lowercase. It's been on my radar for a while, but I could never find a decent approach to fixing it. Hopefully this solution doesn't sacrifice a ton of performance as it simply replaces the lowercase_key ⇔ value HeadersList structure with a lowercase_key ⇔ { key, value } one.

[codecov-commenter]

Codecov Report

Base: 90.24% // Head: 90.27% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (0eea0c9) compared to base (f38fbdc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1784      +/-   ##
==========================================
+ Coverage   90.24%   90.27%   +0.02%     
==========================================
  Files          58       58              
  Lines        5179     5183       +4     
==========================================
+ Hits         4674     4679       +5     
+ Misses        505      504       -1     

Impacted Files	Coverage Δ
lib/fetch/symbols.js	100.00% <ø> (ø)
lib/fetch/headers.js	98.46% <100.00%> (+0.04%)	⬆️
lib/fetch/index.js	83.51% <100.00%> (ø)
lib/fetch/file.js	90.80% <0.00%> (+1.14%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mcollina]

This is not an issue. Headers are case insensitive by the HTTP specs. Lowercasing all of them makes processing faster.

[mcollina]

I'm -1 with this change

[ronag]

Unless the fetch spec says otherwise, I agree with @mcollina. What does Chrome or Deno do?

[mcollina]

RFC2516, 4.2:

HTTP header fields, which include general-header (section 4.5),
request-header (section 5.3), response-header (section 6.2), and
entity-header (section 7.1) fields, follow the same generic format as
that given in Section 3.1 of RFC 822 [9]. Each header field consists
of a name followed by a colon (":") and the field value. Field names
are case-insensitive. ...

[silverwind]

Woudn't it be faster if .toLowerCase is not performed at all?

Generally, I'd lean towards what other implementations do, either let headers through with unmodified case (favoring the sender performance) or lowercase (favoring the receiver performance).

[KhafraDev]

@mcollina there's a WPT for this that all browsers pass, and deno fails - https://wpt.fyi/results/fetch/api/basic/request-headers-case.any.html?label=master&label=experimental&product=chrome&product=edge&product=firefox&product=safari&product=deno&aligned&view=subtest

This also works correctly when using undici.request.

---

Woudn't it be faster if .toLowerCase is not performed at all?

No, since multiple headers can be sent with different casings, we need to lowercase it so that we can do quick lookups. Otherwise we'd have an implementation that was very slow.

Moving the HeadersList structure to an array is a change that I would never do because a map destroys an array in every performance comparison I tried. It's the sole reason that undici's Headers implementation is magnitudes faster than Deno's.

[mcollina]

I can't find a link to that test source. Can you add it?

---

My -1 remain.

lowercasing all headers is one of the reasons undici.request is faster than node http.request.

This change will slow us down further and our performance is already subpar.

Finally, there is absolutely no reason to do this as the current behavior is in line with the HTTP spec, and ultimately the servers needs to understand HTTP, not the fetch() WPT.

[KhafraDev]

The test exists at https://github.com/web-platform-tests/wpt/blob/master/fetch/api/basic/request-headers-case.any.js and I added it in this PR (the wpt.fyi site doesn't correctly link to the github repo unfortunately).

Regarding undici.request, it doesn't lowercase headers automatically, here's a small repro using node's http server & undici.request:

import { once } from 'events'
import { createServer } from 'http'
import { request } from './index.js'

const server = createServer((req, res) => {
  console.log(req.rawHeaders)
  res.end()
}).listen(0)

await once(server, 'listening')

await request(`http://localhost:${server.address().port}`, {
  headers: {
    'ABC-def': 'value'
  }
}).finally(() => server.close())

Where it outputs

[
  'host',
  'localhost:51291',
  'connection',
  'keep-alive',
  'ABC-def',
  'value'
]

[KhafraDev]

Field names are case-insensitive

For this, the RFC doesn't say that headers should lowercase - just that ABC is the same as abc (which is still the behavior in this PR). The only difference is that we don't send the lowercased header to the server anymore, but the first one the user sent. To explain it more, if someone did headers.append('A', 'b') and headers.append('a', 'b'), the server will receive A: 'b, b'.

edit: The title of this PR was/is misleading because I can't figure out a good way of wording it. Sorry if it caused any confusion.

[mcollina]

This kind of processing of Headers is one of the bottlenecks of OutgoingMessage. Let's not repeat the same mistakes.
Given this is cosmetic, let's not do this.

[ronag]

I'm +- 0. When it comes to fetch I personally care more about spec than perf.

[KhafraDev]

This kind of processing of Headers is one of the bottlenecks of OutgoingMessage.

Would you be willing to explain this further? Undici doesn't send lowercase headers to the server, but sends them in the case that they are set (other than fetch currently). This PR matches that behavior, where we send headers as we receive them. See the undici.request example above.

[mcollina]

lgtm, I do not have much time to object to this change and explain why it's not a great idea.