-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
limit-rate.d: this is average over several seconds #7970
Conversation
@@ -17,6 +17,9 @@ Appending 'k' or 'K' will count the number as kilobytes, 'm' or 'M' makes it | |||
megabytes, while 'g' or 'G' makes it gigabytes. The suffixes (k, M, G, T, P) | |||
are 1024 based. For example 1k is 1024. Examples: 200K, 3m and 1G. | |||
|
|||
The rate limiting logic works on averaging the transfer speed to no more than | |||
the set threshold over a period of multiple seconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref: #7965
If the goal here is informing the user that rate limiting split second transfers has no effect then I think you should say that, such as "Rate limiting is averaged over several seconds. For shorter transfers it will have no effect."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorter than what? And it will/might have some effect depending on factors. I'm vague to avoid having to make specific claims that I can't back up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it might have some effect then disregard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information appears to be outdated. At least the send rate is already limited immediately after sending the header, not only after "multiple" seconds. What does that even mean? After a couple of seconds using the entire bandwidth? After more than two, but undefined number of seconds at an undefined rate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limiting logic also appears to have been changed such that it is no longer based on averaging (see 4b86113, #971). What's more, before said change, the documentation used to contain a hint along the lines of what is proposed here:
The given rate is the average speed counted during the entire transfer. It
means that curl might use higher transfer speeds in short bursts, but over
time it uses no more than the given rate.
I suppose it was removed for a reason (because it no longer reflects how the speed limit logic works?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maintain my added sentence is still correct and might help users understand what to expect from this option.
The rate limiting logic works on averaging the transfer speed to no more than | ||
the set threshold over a period of multiple seconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabiankeil What are your thoughts? Is this an accurate description of how the rate limiting works?
wnrph ***@***.***> wrote on 2021-11-09:
@fabiankeil What are your thoughts? Is this an accurate description of how the rate limiting works?
I don't have time to look at the current code right
now but I consider the addition proposed by Daniel
an improvement.
For my Privoxy tests it would be great if I could
limit the rate for header data as well but I suppose
for most use cases it doesn't matter.
While the documentation could make the fact that
the limit isn't applied to the client header data
more obvious it could be argued that this is unrelated
to this pull request.
Fabian
|
No description provided.