feature: add support for request iterator body

[Linkgoron]

continuation of #363

closes #198

[codecov-commenter]

Codecov Report

Merging #848 (7731088) into main (9a7d068) will decrease coverage by 0.18%.
The diff coverage is 99.19%.

@@            Coverage Diff             @@
##             main     #848      +/-   ##
==========================================
- Coverage   99.90%   99.71%   -0.19%     
==========================================
  Files          26       26              
  Lines        2050     2129      +79     
==========================================
+ Hits         2048     2123      +75     
- Misses          2        6       +4     

Impacted Files	Coverage Δ
lib/client.js	99.36% <99.14%> (-0.36%)	⬇️
lib/core/request.js	100.00% <100.00%> (ø)
lib/core/util.js	100.00% <100.00%> (ø)
lib/handler/redirect.js	98.18% <0.00%> (-1.82%)	⬇️
lib/agent.js	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a7d068...7731088. Read the comment docs.

[Linkgoron]

@mcollina Does it make sense to call body.return() on a body Iterator in places that util.destroy(body) is called on a stream (e.g. when throwing in the request constructor etc)? I'm not sure if it makes sense to add the logic to util.destroy, or somewhere else.

[mcollina]

@mcollina Does it make sense call body.return() on a body Iterator in places that util.destroy(body) is called on a stream (e.g. when throwing in the request constructor etc)? I'm not sure if it makes sense to add the logic to util.destroy, or somewhere else.

I think so, yes. wdyt @ronag?

[Linkgoron]

Should the should not follow redirects when using Readable request bodies behaviour (which I assume exists because of "backpressure" or missing some data) also be ported to async-iterators? It's simple enough, I just wasn't sure if it's needed for async iterators as well.

[Linkgoron]

@mcollina Does it make sense call body.return() on a body Iterator in places that util.destroy(body) is called on a stream (e.g. when throwing in the request constructor etc)? I'm not sure if it makes sense to add the logic to util.destroy, or somewhere else.

I think so, yes. wdyt @ronag?

After looking at this a bit more, I'm not 100% sure that it's needed. As Undici gets an async-iterable, the iterable supposedly should only create the resources when iteration begins (i.e. symbol.Iterator or symbol.asyncIterator) are called. The other option is to create an iterator/asyncIterator from the body and immediately call return on it.

[ronag]

Can you keep the existing stream code and make a separate path for iterables?

[Linkgoron]

I broke up the for await...of to manual iteration, so that the iterator could be closed on socket error (which is essentially the same behaviour as the stream body path that destroys the body on socket error). The code is a bit more complicated, as correct handling of return is now needed, so pointers to simplify it are welcome.

[Linkgoron]

I think that I've essentially done what's needed with the implementation. There are some open outstanding issues that I opened that I wasn't sure how to solve, if they're correct or if they're needed (e.g. redirect), and I tried finding and porting all of the stream body tests that I could find and seemed relevant (I looked for createReadStream, new Readable and createReadable), and I also added a few extra tests.

PTAL and tell me if there are any outstanding issues, if something I did was way off in general, or if I should add benchmarks etc.

[ronag]

@Linkgoron I helped out a little

[Linkgoron]

Technically, once the next yield happens, the for...await of will take care of it (as it throws socket[kError])

[ronag]

Yes, but body could be doing a lot of work before the next yield happens.

[Linkgoron]

I tried doing something smarter, but calling return doesn't affect the iterator at all until it yields anyway, so it somehow has to be in userland.

[mcollina]

lgtm