Provide http-config option for curl low-speed-limit/time #5957

lostiniceland · 2018-09-01T08:30:00Z

Provides new override options for curl in Cargo http config-section.

[http]
timeout = 2
low-speed-limit = 5

Test with the following during crate-download
sudo tc qdisc add dev eth0 handle 1: root htb default 11 && sudo tc class add dev eth0 parent 1: classid 1:1 htb rate 8bps && sudo tc class add dev eth0 parent 1:1 classid 1:11 htb rate 8bps

Clear with
sudo tc qdisc del dev eth0 root

rust-highfive · 2018-09-01T08:30:10Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Fixes rust-lang#5717

alexcrichton

Thanks! Could documentation for this option also be added to src/doc?

alexcrichton · 2018-09-02T21:49:26Z

src/cargo/ops/registry.rs

+    if let Some(s) = config.get::<Option<u32>>("http.low-speed-limit")? {
+        return Ok(Some(s));
+    }
+    Ok(Some(10))


Since this always returns Some, perhaps this could return CargoResult<u32> and use .unwrap_or(10) internally?

Removed the Option, but instead of unwrap_or I've set the default in the function. This way, everything is in one place.

alexcrichton · 2018-09-02T21:50:05Z

src/cargo/ops/registry.rs

+        handle.low_speed_limit(config_low_speed_limit)?;
+    }
+    if let Some(config_low_speed_time) = low_speed_time(config)? {
+        handle.low_speed_time(config_low_speed_time)?;


I think this conflicts with http_timeout below, but could it suffice to avoid configuring this manually and letting http_timeout below configure it?

Right. I just didn't notice the option was configured with the http-timeout.

Fixes rust-lang#5717 low-speed-time was removed because the corresponding curl-option is already set via http.timeout. removed unnecessary Option.

alexcrichton · 2018-09-03T17:23:42Z

@bors: r+

Thanks!

bors · 2018-09-03T17:23:43Z

📌 Commit d6cde29 has been approved by alexcrichton

bors · 2018-09-03T17:23:51Z

⌛ Testing commit d6cde29 with merge 19c865c341814e4eba60c834d35ca6ec5fa45001...

bors · 2018-09-03T19:23:51Z

💥 Test timed out

ehuss · 2018-09-03T22:51:17Z

@bors retry

bors · 2018-09-03T22:51:25Z

⌛ Testing commit d6cde29 with merge 86c9c51...

Provide http-config option for curl low-speed-limit/time Fixes #5717 Provides new override options for curl in Cargo http config-section. ``` [http] timeout = 2 low-speed-limit = 5 ``` Test with the following during crate-download `sudo tc qdisc add dev eth0 handle 1: root htb default 11 && sudo tc class add dev eth0 parent 1: classid 1:1 htb rate 8bps && sudo tc class add dev eth0 parent 1:1 classid 1:11 htb rate 8bps` Clear with `sudo tc qdisc del dev eth0 root`

bors · 2018-09-03T23:53:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 86c9c51 to master...

ehuss · 2018-09-04T00:24:49Z

@lostiniceland @alexcrichton I was taking a closer look at this and I was a little concerned. IIUC, this change makes it so that there is no timeout by default (aside from the 30sec connect timeout). If a download hangs midway, it will never time out. Is that intentional?

(As a side note, the docs for timeout is wrong in several ways, I can send a PR later to clarify it.)

lostiniceland · 2018-09-04T06:38:13Z

@ehuss This change keeps the current behavior unless you specify the low-speed-limit. If you set it to 0, it will disable the check completely (see curl-doc). And yes, in this case the download can hang midway I guess.

dwijnand · 2018-09-04T08:49:21Z

Maybe it's best mention the default in the docs rather than the disable.

lostiniceland · 2018-09-04T14:47:46Z

It is mentioned in the docs

low-speed-limit = 0 # Lower threshold for bytes/sec (10 = default, 0 = disabled)

dwijnand · 2018-09-04T14:59:02Z

I mean have it as low-speed-limit = 10.. But maybe I'm just too paranoid of how copy-paste happy people are 😄

lostiniceland · 2018-09-04T15:02:31Z

True. Maybe changing the docs to a non-disabled option is better.
@alexcrichton do I need another issue for this or can I just send in a new PR?

ehuss · 2018-09-04T15:47:25Z

@lostiniceland In the new code, if you don't specify a timeout, then low_speed_time is never set. It defaults to 0 (disabled), so by default there is no longer a timeout. Previously it defaulted to 30 seconds.

dwijnand · 2018-09-04T16:02:53Z

It defaults to 0 (disabled)

Where? I see it defaulting to 10 now.

lostiniceland · 2018-09-04T16:17:15Z

Thanks for the thorough review.

The default low_speed_limit is set regardless of the timeout. So without any timeout a dropping connection should be canceled due to the limit 10bps, except you disable it with 0
https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/registry.rs#L397

It should be exactly as before, just that the default of 10 can be overridden.

EDIT: created #5973 to fix the docs

ehuss · 2018-09-04T16:47:39Z

Where? I see it defaulting to 10 now.

The low_speed_limit (the bytes per second) is 10. The low_speed_time defaults to 0. Both values must be set for the low-speed detection to work.

The default low_speed_limit is set regardless of the timeout.

I'm talking about the low_speed_time, not the limit.

alexcrichton · 2018-09-04T16:50:57Z

Oh yes sorry @ehuss, that was my mistake missing that! @lostiniceland would you be willing to send a PR to re-add the default call to low_speed_time?

lostiniceland · 2018-09-04T17:11:12Z

I will add this to #5973

alexcrichton · 2018-09-04T17:12:49Z

Ok, thanks!

dwijnand · 2018-09-04T18:12:27Z

Man I'm blind..

rust-highfive assigned alexcrichton Sep 1, 2018

lostiniceland force-pushed the 5717-curl-low_speed branch 2 times, most recently from 28fb126 to 7ed4db1 Compare September 1, 2018 18:39

Provide http-config option for curl low-speed-limit/time

0279123

Fixes rust-lang#5717

lostiniceland force-pushed the 5717-curl-low_speed branch from 7ed4db1 to 0279123 Compare September 1, 2018 22:30

alexcrichton reviewed Sep 2, 2018

View reviewed changes

Provide http-config option for curl low-speed-limit

d6cde29

Fixes rust-lang#5717 low-speed-time was removed because the corresponding curl-option is already set via http.timeout. removed unnecessary Option.

bors merged commit d6cde29 into rust-lang:master Sep 3, 2018

lostiniceland deleted the 5717-curl-low_speed branch September 4, 2018 06:15

ehuss mentioned this pull request Sep 5, 2018

Update Cargo and RLS rust-lang/rust#53935

Closed

ehuss added this to the 1.30.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide http-config option for curl low-speed-limit/time #5957

Provide http-config option for curl low-speed-limit/time #5957

lostiniceland commented Sep 1, 2018 •

edited

Loading

rust-highfive commented Sep 1, 2018

alexcrichton left a comment

alexcrichton Sep 2, 2018

lostiniceland Sep 3, 2018

alexcrichton Sep 2, 2018

lostiniceland Sep 3, 2018

alexcrichton commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

ehuss commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

ehuss commented Sep 4, 2018 •

edited

Loading

lostiniceland commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018

ehuss commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018 •

edited

Loading

ehuss commented Sep 4, 2018

alexcrichton commented Sep 4, 2018

lostiniceland commented Sep 4, 2018 •

edited

Loading

alexcrichton commented Sep 4, 2018

dwijnand commented Sep 4, 2018

Provide http-config option for curl low-speed-limit/time #5957

Provide http-config option for curl low-speed-limit/time #5957

Conversation

lostiniceland commented Sep 1, 2018 • edited Loading

rust-highfive commented Sep 1, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Sep 2, 2018

Choose a reason for hiding this comment

lostiniceland Sep 3, 2018

Choose a reason for hiding this comment

alexcrichton Sep 2, 2018

Choose a reason for hiding this comment

lostiniceland Sep 3, 2018

Choose a reason for hiding this comment

alexcrichton commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

ehuss commented Sep 3, 2018

bors commented Sep 3, 2018

bors commented Sep 3, 2018

ehuss commented Sep 4, 2018 • edited Loading

lostiniceland commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018

ehuss commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 4, 2018 • edited Loading

ehuss commented Sep 4, 2018

alexcrichton commented Sep 4, 2018

lostiniceland commented Sep 4, 2018 • edited Loading

alexcrichton commented Sep 4, 2018

dwijnand commented Sep 4, 2018

lostiniceland commented Sep 1, 2018 •

edited

Loading

ehuss commented Sep 4, 2018 •

edited

Loading

lostiniceland commented Sep 4, 2018 •

edited

Loading

lostiniceland commented Sep 4, 2018 •

edited

Loading