Fixes #7126.

[dom96]

Other ideas of how this should be implemented are welcome. I'm not entirely happy about my solution:

• Synchronous HttpClient has a different API for the streaming of the body to the AsyncHttpClient which is a shame.

• Two callbacks to stream data feels ugly to me.

[endragor]

Have you considered making FutureStream more flexible? So that read(futureStream) would actually perform an operation on the underlying socket, instead of waiting for data in the queue? It could be implemented similarly to synchronous streams - implementations can supply their own read and write procedures/closures.

So AsyncHttpClient would return a stream that is attached to its socket, and read would perform async recv on the socket. Similarly, HttpClient could return a blocking stream attached to its blocking socket. I don't think allowing to read data from HttpClient with a pair of callbacks makes a lot of sense. If people want async operations, they should go with AsyncHttpClient.

[dom96]

Have you considered making FutureStream more flexible? So that read(futureStream) would actually perform an operation on the underlying socket, instead of waiting for data in the queue? It could be implemented similarly to synchronous streams - implementations can supply their own read and write procedures/closures.

I haven't, but that should be considered outside this PR. In all honesty, I'm happy with the current implementation.

Similarly, HttpClient could return a blocking stream attached to its blocking socket. I don't think allowing to read data from HttpClient with a pair of callbacks makes a lot of sense. If people want async operations, they should go with AsyncHttpClient.

In some cases users might like to use the synchronous implementation with threads. I don't think I can return a blocking stream, unless I prevent this functionality.

[endragor]

Could you clarify how it would work with threads, given the httpclient module uses GCed references, Nim has thread-local GCs, and it's unsafe to access objects from other thread's heap? If you mean they may want to receive data on a separate thread and send it to another thread, then blocking streams should work fine with this use case.

Also, this implementation for async client again doesn't really allow to process data in chunks. asyncCheck parseBody still means it will be receiving the whole response body into memory. But the point of chunked processing is that you may be getting gigabytes of data and you simply cannot keep it in memory - you want to get a chunk, process it (compress and send further, or just send further to a different stream, such as file or a socket) and do so in loop.

[dom96]

If you mean they may want to receive data on a separate thread and send it to another thread, then blocking streams should work fine with this use case.

That is what I mean. I'm not sure it's possible to implement chunked processing of data with blocking streams though. If it is then that is a good way to do it.

Also, this implementation for async client again doesn't really allow to process data in chunks.

It does but only if you process the data immediately. Is that really a problem? Delaying the processing of the HTTP body sounds like it would cause issues.

[dom96]

So I tried to use Stream for the synchronous HttpClient but I was not able to do so successfully. I quickly realised that I would need to save the data inside the stream which would result in high memory usage... so there really doesn't seem to be a better way than the callbacks.

[endragor]

What I meant is a blocking socket stream that directly accesses the socket. Writing into string stream from httpclient and then making user read from it feels hacky to me, both in async and sync versions, as it removes control from the user.

You may want to delay reading if you would like to preserve bandwidth. For example, you are doing multiple simultaneous requests with different priorities and you want to throttle low-priority requests based on available bandwidth. Or you are making a file download utility that has download speed limit option (see --limit-rate in wget/curl, for example).

[dom96]

Alright. That's a worthy use case. Let's change FutureStream to be more like Stream.

That's going to be a much bigger job.