Read scenario fixed #2
Conversation
src/fcurl.c
Outdated
| return 1; /* no buffer data left and transfer complete == done */ | ||
|
|
||
| if(!h->transfer_complete) { | ||
| do { | ||
| mc = curl_multi_wait(h->mh, NULL, 0, 5000, &numfds); | ||
| if (h->paused) { | ||
| /* Unpause + no 'wait' because it was paused with excess data */ |
There was a problem hiding this comment.
Shouldn't this be indented two spaces?
There was a problem hiding this comment.
Correct, it should! My bad, "styling" issue, I used 8 char TABs instead of spaces as in the original code. Fixed it with dec60b9
| @@ -29,6 +29,7 @@ size_t fcurl_read(void *ptr, size_t size, size_t nmemb, FCURL *stream); | |||
| size_t fcurl_write(const void *ptr, size_t size, size_t nmemb, | |||
| FCURL *stream); | |||
| int fcurl_flush(FCURL *stream); | |||
| int fcurl_eof(FCURL *stream); | |||
There was a problem hiding this comment.
This doesn't really "fix the read scenario" but is unrelated.
There was a problem hiding this comment.
Sorry, possibly mixed up with PR title that gets all the changes, the commit says "Provides a definition for fcurl_feof", which was missing. Should have made 2 PRs!
src/fcurl.c
Outdated
|
|
||
| return ret; | ||
| /* Still does not make difference between EOF and errors */ | ||
| return (h->transfer_complete && 0 == h->b.used); |
There was a problem hiding this comment.
Yes, a basic one, still does not make difference eof/error, but is needed in a classic while(eof) { read; write; } loop. Without an "eof", the "read scenario" (in which I include the loop, so it is a question of how you define "scenario" !) does not work. But agreed, a single read would work without this addition.
|
Sorry, I'm not completely at ease with PR! I should probably have made 2 different PR for the first 2 commits. |
|
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. |
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?
... and then do unlimited amounts of data buffered? That seems insane? |
|
Maybe my understanding is incorrect.
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. You are correct, buffering is another matter, and probably not needed. |
It also stops calling
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: 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! |
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.
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! |
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