curl
Read scenario fixed
Fixed a bug in the callback (memcpy offset in the user buffer was missing) plus minor things to make it work for a "read" scenario
Sorry, I'm not completely at ease with PR! I should probably have made 2 different PR for the first 2 commits. There is a 3rd commit not in the PR (or is it? it was not my intention!) since it is not "fix" but a different and shorter algorithm that does not pull all the data into the client and uses "pause/unpause" instead to let libcurl perform its magic.
Discussion: the pause/unpause algorithm could have a negative impact when used with http/2, because current libcurl behaviour is (as far I understood the code) to close/open the http/2 window when pausing/unpausing. Would it be an option to be able to "pause" without changing the http/2 window (CURLPAUSE_LOCAL_RECV or CURLOPT_KEEP_WINDOW for http/2)? Is it already there and I missed it in the code? Wouldn't then nghttp2 (or libcurl) do the buffering? If the client reads too slowly, in the end it will be the same because the http/2 window will end up being full whether it has been closed or not. So the change would be useful only when you expect that the client's code is fast enough with the reads.
could have a negative impact when used with http/2, because current libcurl behaviour is (as far I understood the code) to close/open the http/2 window when pausing/unpausing.
I don't understand this comment. Are you saying that curl should maintain the window at the current size even though the transfer is paused? What would be the purpose of that and why?
Would it be an option to be able to "pause" without changing the http/2 window (CURLPAUSE_LOCAL_RECV or CURLOPT_KEEP_WINDOW for http/2)?
... and then do unlimited amounts of data buffered? That seems insane?
Maybe my understanding is incorrect. I understand the semantics of curl_easy_pause() as such:
- The client (caller) asks the library not to send any more data at the moment, which means stop invoking the write_callback.
- It does not say that the server itself should be ordered to stop sending data (which is currently implied with http/2).
I don't know http/2 well enough to differentiate the impact if libcurl only stops calling recv() on the socket or really sends the close window. If you think that makes no difference, no change is required!
You are correct, buffering is another matter, and probably not needed.
The client (caller) asks the library not to send any more data at the moment, which means stop invoking the write_callback.
It also stops calling recv() on the socket if there's no other transfers going on over the same socket.
It does not say that the server itself should be ordered to stop sending data (which is currently implied with http/2).
TCP and HTTP/2 have "windows" for how much data the server is allowed to send and the server can keep on sending that amount. curl cannot change that. For TCP, that window handling and buffering is done in the kernel. For HTTP/2 and HTTP/3, the handling of that data needs to be done in the application if that data is read off the socket. curl cannot tell the server to "stop sending", but it can cease to update the window so that the server will eventually stop sending when the window is all used up.
Understood, I remember my "old" TCP/IP class with the anticipation window to avoid network latency!
Separation of concern principle.
The caller's perspective when using curl_easy_pause() is that it does not want to be called on the callback at the moment.
The library should do "what's best", and that is not the concern of the caller... unless the library can't determine "what's best" without a clue from the caller, in which case there should be a way to give that clue.
With the current pause/unpause code, if the caller uses "reasonable" buffers like 32k on top of https, in fact pause is never called because the 16k TLS buffer fit nicely in the caller's buffers.
But if you try with "stupid buffers" like say 12371 byte, what happens is: 1- write_callback(16k) 2- caller consumes 12371 bytes 3- caller issues "pause" 4- (http/2) libcurl sends window close 5- caller writes data received from libcurl to local file (or whatever) 6- caller reads next buffer which issues "unpause" 7- (http/2) libcurl sends window re-open 8- write_callback(16k) -same as 1 because it was "paused" 9- caller consumes the 4013 bytes remaining 10- write_callback(16k) -new buffer 11- caller consumes 8358 bytes (its buffer is now full) 12- goto 3 (while not eof)
So if this close/reopen to the server would have an effect on network performance, shouldn't there be a way the caller tells libcurl how to best do it's network magic?
So indeed, as you phrase is: "cease to update the window", if given the clue. Something like CURLPAUSE_SHORT_RECV, if that could be useful for the library, when the caller anticipates step 5 above is "short".
[Edit note:] I have not made a "benchmark", when the client is writing quickly enough, to see whether this close/re-open in the case of http/2 would have a noticeable network performance effect, or none at all. I would have had to change curl's code for that, and network is the wisdom of the library, not of the client's code!
4- (http/2) libcurl sends window close
No. There's no such thing. curl has already announced to the server that the window is of size N. It can't shrink the window, it can only cease to increase it, which means the server can keep sending the number of bytes left in the window.
7- (http/2) libcurl sends window re-open
There's no "reopen". When the application starts reading from the stream again, curl will give the server more window again to start sending data.
Perfect, so that should be all fine with http/2 too, thanks for the explanations!