fetch: make readAllBytes sync

[Uzlopak]

Because we cant access the ReadableStreamDefaultReader logic inside node core to provide our own reader with the specified logic as mentioned in the spec. Before readAllBytes is async, which is confusing because we provide two callbacks to it. Now it reads more natural for me.

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[KhafraDev]

There is no reason to change it

[mcollina]

This allocates way less promises and I expect to improve performance.

I don't really understand how this work tho. What if part of the body is not available?

[Uzlopak]

If body is null or invalid then it would break already in an earlier stage in the function which calls readAllBytes, because we call getReader() on the body which can throw an error. We expect in readAllFiles the generated Reader.

The only place we can error is when the reader is locked, so that read will throw. Other than that i dont see any code path possible to result in an error.

[Uzlopak]

LOL, I actually found a bug in the sri code. I was wondering why setting the rejection handler in readLoop is not working.

[mcollina]

What about if a large file is transmitted? Is fetch already buffering all of that in memory?

[Uzlopak]

Only if you use one of those methods from the bodyMixing, like .text()``,.json() and so on. If you know that you expect a huge file you should stream it anyway.

For streams you have that logic directly in mainFetch called incrementalReadBody or so. There we use alot of magic like async generators.

[gurgunday]

Can you bench it somehow? I think it's a pretty good change if there's a significant improvement

[Uzlopak]

@gurgunday

What I need is a way to count the generated Promises.

[gurgunday]

I use https://github.com/bengl/count-promises for that

[gurgunday]

lgtm

[mcollina]

lgtm

[mcollina]

@Uzlopak this now conflicts. can you use #4349 (comment) to count the promises?

@KhafraDev this seems to allocate way less promises than async/await combo.

[Uzlopak]

My change reduces according to the "test" that the amount of promises is reduced by 1 per fetch call

[KhafraDev]

1 less promise?

[Uzlopak]

yes, 1 less promise. We create about 50 promises for a fetch call.

See the other PRs regarding removing async/promises. Maybe we can remove overall about 10 promises.

[gurgunday]

Just my 50c but I think this and other PRs that remove unnecessary promises are ultimately beneficial and they add up