fix #3337

[KhafraDev]

fixes #3337 (confirmed with the repro op gave, couldn't repro locally)
might fix #3328 but not sure

Effectively reverts f979ec3

Adding this to the slight-spec-deviation-that-ends-up-fucking-up-things list

[Uzlopak]

@KhafraDev
Tests fail?

[KhafraDev]

We do need a test for this still, but considering the severity of this I think it was appropriate to land. Note that this is only a revert and now the reverted sections are verbatim with the spec.

From the linked issue: "Reproducible on Node 22.3 and NOT on 22.2."

• node v22.2 uses undici v16.3.0
• node v22.3 uses undici v16.8.0

I'm assuming that this is not an issue in the undici dispatcher api, otherwise it would have likely been noticed much earlier. The only change done with fetch's body between those two versions was the commit I linked (f979ec3).

Furthermore, I will be blocking any change done to the specs we implement if they attempt to skip or modify the spec in some way. I think we should accept the limitations of these specs, instead of attempting to "node-ify" them.

[KhafraDev]

I've created a CSV of pretty much every bug fix in fetch over the last 4 months (as far back as github would take me, since I proved my point I didn't bother going back further).

Out of 13 bug fixes:

• 9 of them were caused by deviating from the spec.
• 2 of them are legitimate bugs that occurred despite us following the spec.
• 1 of them was effectively a typo.

There is a chance I missed some, but there are a disproportionate amount of bug reports and bug fixes revolving around deviations from the spec.

[mcollina]

@KhafraDev I don't think this specific test was meant to break the spec, and it didn't, otherwise our WPTs would have caught it, right?

I think we can all agree on these 3 goals:

1. improve the performance of fetch()
2. don't break our users
3. break the spec only as a last resort

If the current set of tests cannot catch regressions, then the problem is in the test coverage.

[KhafraDev]

I don't agree, fetch is a web spec and we should do our best to implement the spec. There are better suited apis for people who need better performance. Otherwise, I can't help but agree with what ljharb said initially about fetch (paraphrasing) "if it's not compliant with the spec, why not rename it to something else?".

[KhafraDev]

I would like to mention that I wholeheartedly do agree w.r.t test coverage. Our current tests are not enough to safely deviate from the spec, so we should refrain, from all means possible, from doing so.

[mcollina]

I don't agree, fetch is a web spec and we should do our best to implement the spec.

I think we are in agreement on this.

There are better suited apis for people who need better performance. Otherwise, I can't help but agree with what ljharb said initially about fetch (paraphrasing) "if it's not compliant with the spec, why not rename it to something else?".

We disagree here. fetch() is extremely popular, and we should improve its performance compared to the rest of the libraries. The two goals are not at odds, and I don't understand why you claim they are.

I would like to mention that I wholeheartedly do agree w.r.t test coverage. Our current tests are not enough to safely deviate from the spec, so we should refrain, from all means possible, from doing so.

I think our tests are not enough to make many changes to fetch(), independently of the spec. The given PR was not "violating/breaking" the spec in any deliberate way.
It didn't break any wpt, and so far I haven't been able to reproduce the bug at all.

[ChALkeR]

This case wasn't cloned in shouldClone = true case in the same sense that the call in arrayBuffer() expected, as Buffer.concat returns a pooled Buffer, and arrayBuffer() returned just a .buffer of it

[KhafraDev]

The given PR was not "violating/breaking" the spec in any deliberate way.

It wasn't a deliberate change, but it absolutely did violate the spec in any sense of the definition. If the spec tells us to clone the buffer, and we do not clone it, we have issues like this. The PR willingly skipped the clone, knowing full well that the spec tells us to clone the buffer.

The spec:

The arrayBuffer() method steps are to return the result of running consume body with this and the following step given a byte sequence bytes: return the result of creating an ArrayBuffer from bytes in this’s relevant realm.

[KhafraDev]

The two goals are not at odds, and I don't understand why you claim they are.

Recent changes have shown that following the spec - as stringently as we once did - comes second to performance. In another issue you said that support node.js use cases comes before following the spec - "IMHO the only deviation from the spec we should be doing is to support fetch() use cases that our users expect to be able to do in Node.js and they are blocked in Browsers". I am assuming that performance is one of these use cases (... and diagnostics channel support...).

I think our tests are not enough to make many changes to fetch(), independently of the spec

I agree. The WPTs don't cover everything, not that I expect them to, but what they do cover is the text in the spec.
For example, before submitting a PR to a WHATWG spec repo, they ask if you've 1) filed implementation issues, and 2) if you've opened a PR in the wpt repo that adds tests. That is why it is so important to follow the spec, instead of skipping steps for whatever reasons.