Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First buffer upload seem to ignore the send speed #5788

Closed
Togtja opened this issue Aug 6, 2020 · 5 comments
Closed

First buffer upload seem to ignore the send speed #5788

Togtja opened this issue Aug 6, 2020 · 5 comments
Labels

Comments

@Togtja
Copy link

@Togtja Togtja commented Aug 6, 2020

I did this

When uploading files to an SFTP server, I limited the CURLOPT_MAX_SEND_SPEED_LARGE to 1024, and set the CURLOPT_UPLOAD_BUFFERSIZE to it's minimum of 16kb. When I upload a file of 25855 bytes it finished the first 16384 bytes nearly instantly, then takes some time for the rest. It spends totally 16 seconds uploading something that should take 25855/1024 = 25 seconds give or take. Leaving the CURLOPT_UPLOAD_BUFFERSIZE to the default 64kb makes the entire upload happen instantly.

I also noticed something similar for download as well, if I set CURLOPT_BUFFERSIZE to say 131072 bytes (CURL_MAX_READ_SIZE/4), the entire download happens instantly. If I lowered CURLOPT_BUFFERSIZE to the same as CURLOPT_MAX_RECV_SPEED_LARGE it would take the expected time to download.

I expected the following

I expect that even if the buffer size is larger than the max speed, the max speed would be upheld.

curl/libcurl version

curl 7.71.1 (Windows) libcurl/7.71.1 OpenSSL/1.1.1g libssh2/1.9.0_DEV
Release-Date: 2020-07-01
Protocols: dict file ftp ftps gopher http https imap imaps ldap pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS HTTPS-proxy IPv6 Largefile NTLM SSL

operating system

Windows 10 Enterprise

@bagder
Copy link
Member

@bagder bagder commented Aug 7, 2020

Clearly, when libcurl's been instructed to do the transfer slower (in bytes/sec) than the size of the upload/download buffer, it should rather limit the amount data to handle to the capped amount in order to better acknowledge the setting in the first network read/write.

I think this is just an oversight as it only affects the first network operation and capping network speed has always been about being "roughly right over time", and not an exact science.

Are you interested in writing a patch for this?

@Togtja
Copy link
Author

@Togtja Togtja commented Aug 7, 2020

I am willing to give it a shot, if I can get pointed to the right direction in the source code.

@bagder
Copy link
Member

@bagder bagder commented Aug 7, 2020

Curl_fillreadbuffer reads data into the upload buffer

The primary point in the code that reads data off the network in a download state is when readwrite_data() calls Curl_read.

@Togtja
Copy link
Author

@Togtja Togtja commented Aug 10, 2020

I played around with it today, and as far as I can tell, CURL, calls Curl_fillreadbuffer and fills the buffer, then wait for an appropriate about of time so that the limit-rate get correctly averaged, which I suppose work fine in household networks etc.
However, I am dealing with a very limited pipe, of something like 20kb/s on bad days lower, where a 64kb filled buffer stop all communication for 3 seconds. As alluded to in my issue, the temporary fix for this is to set CURLOPT_MAX_SEND_SPEED_LARGE and CURLOPT_UPLOAD_BUFFERSIZE to the same value (For download CURLOPT_MAX_RECV_SPEED_LARGE and CURLOPT_BUFFERSIZE). As I am no network expert, I do not know if this is intended behaviour for buffer to:
Ask the server to fill the buffer, then wait buffer/speed seconds and ask for the next buffer

This seems to be supported when I run the curl binary "Current Speed" stay at 0, for a long time, the jumps up for some few seconds, then back to 0

@bagder
Copy link
Member

@bagder bagder commented Aug 12, 2020

Yes that's how it currently works which has the effect on the first recv/send you described. Limiting the buffer sizes is of course a mitigating method.

@bagder bagder added the libcurl API label Aug 13, 2020
bagder added a commit that referenced this issue Aug 14, 2020
... in particular what happens if the maximum speed limit is set to a
value that's smaller than the transfer buffer size in use.

Reported-by: Tomas Berger
Fixes #5788
@bagder bagder closed this in d491916 Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.