Coverage decreased (-0.008%) to 73.233% when pulling 5d905db on paulharris:test-speedcheck into 7fffe97 on curl:master.

the initial upload data biases the current speed calculation, and the speeder[] array updates will ignore all the downloaded data progress until the total downloaded bytes is more than the total uploaded bytes. Not a good approach

Nah, that's really stupid. I think the "current speed" should be the maximum speed in either direction for this to work properly. Possibly it should be both directions?

the speeder[] array can be updated on occasions (by Curl_pgrsUpdate) when the Curl_speedcheck() function is not subsequently called

I think we can fix this pretty neatly by moving the call to Curl_speedcheck to happen from within Curl_pgrsUpdate() instead and remove the invokes from everywhere else. Should make it harder to do this by mistake. Agree?

@paulharris: what about my suggested fix in #1559 as a first step?

I haven't checked what you've done yet, but I wanted to fire off an email while you are online.

I was thinking about the approach to solve, and I was thinking...

SPEEDCHECK THRESHOLD

Allow Curl_speedcheck to remember the last total transferred bytes (up+down) and the last "passed threshold time". I don't see why we should use any of the update's transfer speeds at all.
I interpret speedcheck's "average speed within time" to mean "must transfer LIMITxTIME bytes within TIME", so we just need to reset the timer whenever it has transferred enough bytes.
So, if LIMIT=100 bytes/sec, TIME=20 sec, then
THRESHOLD=100x20=2000 bytes must be transferred within a 20 sec window.
You record the start time and the total_transferred at that time.
Each time speedcheck() is called, it checks if
(TRANSFERRED_NOW - TRANSFERRED_START) >= THRESHOLD
if true, then reset... record the start_time and total_transferred again.
If false, and (NOW - START_TIME) >= TIME then its too slow.

WHEN TO CALL SPEEDCHECK

Curl_pgrsUpdate is called in about 20 different places, but Curl_speedcheck is called in only < 8 places.
I don't know when Curl_speedcheck should be called, but putting it directly in prgsUpdate might be a significant change.
eg, we probably don't want to be checking for low speeds when negotiating with the proxy or doing name lookups.

CURRENT SPEED

Instead of speeder, perhaps we should just be adding the dl and ul speeds together?
ie current_speed = ulspeed+dlspeed ?
Why is that not a good approach?
My guess is that ul/dlspeed is a longer term average, whereas current_speed is supposed to be the speed now.
In which case, why not use a rate smoothing style calculation, like this one I use (edited as example)...

void init() {
   state.window_len = 0;
   state.ul_rate_average = 0;
   state.dl_rate_average = 0;
}

void update( Time now_utc, Bytes ulnow, Bytes dlnow ) {
   // ignore updates after a zero time gap!
   if (now_utc > state.last_time_utc) {
      static const double max_window_secs = 60;

      double secs = secs_diff(now_utc-state.last_time_utc);
      double ul_rate = (ulnow-state.last_ul) / secs;
      double dl_rate = (dlnow-state.last_dl) / secs;

      // normally, smoothing = 1/200 ie 1 part in X minutes,
      // but our 'secs' could be different to 1...
      state.window_len = std::min(max_window_secs, state.window_len + secs);
      double smoothing = std::min(1.0, secs / state.window_len);  // don't let ratio be > 1
      state.ul_rate_average = ul_rate * smoothing + (1-smoothing) * state.ul_rate_average;
      state.dl_rate_average = dl_rate * smoothing + (1-smoothing) * state.dl_rate_average;

      state.last_time_utc = now_utc;
      state.last_ul = ulnow;
      state.last_dl = dlnow;
   }
}

Its not perfect, I need to improve it a bit,
but it can be tweaked to better bias towards more recent transfer rates and for a smaller total window.

Then we would have a "total dl speed" (the Curl dlspeed as its implemented now)
and a "recent dl speed" (ie what speeder[] gets us)
and the total speeds are just the dl/ul speeds added together.

re: @paulharris: what about my suggested fix in #1559 as a first step?
I've commented in #1559 directly. I'll continue my approach on my branch now, lets see where it leads...

I've updated this with my suggested implementation.
Note that I have not changed where speedcheck is called, as I don't think I want to call it when its doing name lookups or the connection is just getting started, or I'm not sure where else...

I'll have to leave that to you to determine what is the best course of action in that regard.

At the moment, I figure it'll "be ok" since it'll "probably" get called as often as required, but I can't be sure.

Regarding the definition, the "R bytes/sec over T time" is a bit confusing.

If I had a time machine, I'd suggest just using "N bytes over T time".

At the moment, the minimum rate I can specify over a 60 second window is 1 byte/sec.
What if its a longpoll connection, and I want it as low as 1 byte/minute ?

Or 100 bytes / 2 minutes ? Minimum rate over 2 minutes possible is 120 bytes (1bps)

You can see someone had the same confusion here (I'm not involved in this project, I just found it via search):
collectd/collectd#985

Quote:
- rename LowLimitBytesPerSec to LowSpeedLimit.
- make LowSpeedLimit refer to bytes per second rather than bytes per
  Interval.

The reason I'm using the low-speed limit is actually to avoid using timeouts in longpoll connections, while at the same time watching for broken connections that Curl doesn't detect (by design, it seems?).
From: collectd/collectd#752

I've been seeing a problem in some production systems using write_http where cURL hangs forever (and I'm talking weeks) waiting for a request to complete, because the TCP socket according to the client remains open while the server considers it have been broken by network instability.

Looking at the code (and libCURL docs) it seems that there is no default timeout, and waiting forever is expected behaviour in this case.

I get the impression that this is the only current patch which would address this issue? Is that correct or have I missed some code which introduces some sort of timeout to the HTTP request?

Is the above the correct design, from Curl's point of view?
I see a few reasons for this here: https://stackoverflow.com/questions/14081696/curl-stop-working

Sorry if this is polluting this discussion...

Coverage increased (+0.004%) to 73.221% when pulling fce8e99 on paulharris:test-speedcheck into 844896d on curl:master.

Another question... see here:
https://forum.kodi.tv/showthread.php?tid=131873

Specifically,

* CURLOPT_LOW_SPEED_LIMIT combined with a restricted buffer will cause unexpected abort of playback in media when a pause or a skip back causes throttling of the transport below 1bps.

In short, they are streaming video. They pause the video (or the buffer becomes full) so they throttle the network speed.
But, the low speed limit is still in place and triggers a connection-lost.

In this scenario, shouldn't the Curl user adjust the low-speed-limit to match the throttled speed desired?

And if so, CAN the Curl user adjust the low-speed-limit as the code currently stands?
I believe if the user changes the thresholds, the code (as I've written it) will simply test against the new thresholds at the next call, and work as expected.

This might be related to the not-always-calling Curl_speedcheck() question...
https://stackoverflow.com/questions/33778157/curl-tftp-transfer-timeout

First, I think we should focus on getting the existing functionality done right. I don't think it is valuable to scan a lot of random misunderstandings or others repeating our bugs before then. Once we believe we have ironed out our current problems, then I think we can start to see what we can do for the remaining problems (if possible).

The low speed timeout was never meant to be the all-covering solution for everyone. It is just one way for applications to kill almost idle connections. Applications can and should still implement their own logic if they want more fancy algorithms.

So, if LIMIT=100 bytes/sec, TIME=20 sec, then THRESHOLD=100x20=2000 bytes must be transferred within a 20 sec window.

Those first 20 seconds are easy - but what happens on the 21st second? Do you restart the counter at second 20 and wait another 20 seconds so it won't abort this connection until second 40?

By checking the speed every second, the situation where 2000 bytes come the first second and is then followed by 19 totally stalled seconds is detected at second 21 where it actually reaches 20 seconds in a row at less than 100 bytes/sec...

I'm not sure what you're suggesting. If we ignore the problem #1559 is trying to fix, what is this new code bringing to libcurl that the existing code doesn't already handle?

I think the current code has several advantages compared to your version, including that the "current speed" is based on the last 5 seconds only. Limiting the speed math to the last N seconds is hard to do without using an array like this.

In short, I think your changes in #1559 are ok,
but the description of the limit doesn't match what it does.
If we change the description then I think its fine.

N = average num bytes/sec lower limit
T = num seconds to tolerate low speed

The current description implies (to me):

Over T seconds, the connection should be operating with an average speed of N bytes/sec.
That implies that over T seconds, N*T bytes must be transferred, or else its too slow.

The description should be changed to something more like...

The connection is defined as too slow if its total speed is lower than N bytes/sec averaged over a 5 second window.
A slow connection will be tolerated for T seconds, after which it is aborted with a CURL_OPERATION_TIMEOUT code.

Alternatively: The connection will be aborted (due to low speed) if it T seconds pass without 5*N bytes uploaded or downloaded within a 5 second window.

More thoughts

Ok, so the current speed is based on the total transferred in the last 5 seconds (divided by 5).
Each time Curl_pgrsUpdate() is called, the current speed is updated IF at least 1 second has passed.

Then, Curl_speedcheck() should be called at least once a second (right?)
It is not called every time update is called, yet, we haven't tacked that. Might be different for different protocols.
If speedcheck isn't called in one of those seconds, we might miss the connection's one brief moment of being fast enough. I could manufacture an edge case, but I don't think its required.

You seem to be hung up and not agree that we need more than one second to get an accurate "current speed". I don't think the fact that it uses the last 5 seconds to get that information is something we need to hammer the user with, it is just an implementation detail for how it gets the current speed (I'm concerned that using less will get very unstable number and thus instead cause other issues). And it uses less than 5 seconds the first 5 seconds of a transfer. Therefore, i don't think your suggested updated descriptions improve matters very much.

Then, Curl_speedcheck() should be called at least once a second (right?)

it should. But it is a fairly new requirement so I won't be surprised if there's some case where it doesn't happen. We should probably add a debug-build condition that checks and warns if that happens. Or have you figured out any situation where you know it happens?

I agree we need more than one second, that's logical.
5 seconds seems fine too. I think that is all good, and better than my longer-term-speed approaches.
And I agree my suggested description is not ideal.

My confusion (and I think others) comes from the wording of what an "average speed" is. Current description implies that the average speed over the whole time window should be above the limit.
ie for LIMIT=L bytes/sec, the minimum total bytes transferred over T seconds is T * L

That isn't the case.

The case is that there only needs to be one second in the time window where the "current speed" is above the limit.
ie for LIMIT=L bytes/sec, the minimum total bytes transferred over T seconds is L

Regarding the question regarding whether speedcheck is always called every second, I was thinking of adding some checks, ie when prgsupdate is called AND the speeder is updated, has speedcheck been called since the last speeder update?
But I don't see any other tests that would execute long enough to check the various protocols and scenarios,
And I am only using HTTP.
Who can run the library/exe with the tests enabled?

Is this PR still valid or does #1567 replace this?

#1567 replaces it, in terms of the commit.
I'd still like to see some change to the description.
Would you like to finish that discussion here or move it to a new Issue ?

By the way, you are doing a great job with such a complex library.
I have more suggestions/fixes in the pipeline.

Would you like to finish that discussion here or move it to a new Issue ?

Due to the slight shift in focus from this PR, I think that's better taken to a new PR/issue. Makes it easier for everyone to keep track of the exact topic.

you are doing a great job with such a complex library

Thank you!

Let's consider this PR done. Open a new one if there are more/remaining things to fix.