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

Give a better error message when crates.io requests time out #6936

Merged
merged 3 commits into from May 14, 2019

Conversation

Projects
None yet
6 participants
@sgrif
Copy link
Contributor

commented May 13, 2019

crates.io is hosted on Heroku, which means we have a hard 30 second time
limit on all requests. Typically this is only hit when someone is
attempting to upload a crate so large that it would have been eventually
rejected anyway, but it can also happen if a user is on a very slow
internet connection.

When this happens, the request is terminated by the platform and we have
no control over the response that gets sent. This results in the user
getting a very unhelpful error message from Cargo, and some generic
error page HTML spat out into their terminal. We could work around this
on our end by adding a 29 second timeout somewhere else in the stack,
but we have a lot of layers that buffer requests to protect against slow
client attacks, and it'd be a pretty decent amount of work. Since we
eventually want to switch over to having Cargo do the S3 upload instead
of us, it doesn't make sense to spend so much time on an error scenario
that eventually will go away.

I've tried to keep this uncoupled from crates.io as much as possible,
since alternate registries might not be hosted on Heroku or have the
same restricitions. But I figure "a 503 that took more than 30 seconds"
is a safe bet on this being hit. If we're ok with coupling this to
crates.io, I'd like to include "If your crate is less than 10MB you can
email help@crates.io for assistance" in the error message.

Ref rust-lang/crates.io#1709

Give a better error message when crates.io requests time out
crates.io is hosted on Heroku, which means we have a hard 30 second time
limit on all requests. Typically this is only hit when someone is
attempting to upload a crate so large that it would have been eventually
rejected anyway, but it can also happen if a user is on a very slow
internet connection.

When this happens, the request is terminated by the platform and we have
no control over the response that gets sent. This results in the user
getting a very unhelpful error message from Cargo, and some generic
error page HTML spat out into their terminal. We could work around this
on our end by adding a 29 second timeout *somewhere else* in the stack,
but we have a lot of layers that buffer requests to protect against slow
client attacks, and it'd be a pretty decent amount of work. Since we
eventually want to switch over to having Cargo do the S3 upload instead
of us, it doesn't make sense to spend so much time on an error scenario
that eventually will go away.

I've tried to keep this uncoupled from crates.io as much as possible,
since alternate registries might not be hosted on Heroku or have the
same restricitions. But I figure "a 503 that took more than 30 seconds"
is a safe bet on this being hit. If we're ok with coupling this to
crates.io, I'd like to include "If your crate is less than 10MB you can
email help@crates.io for assistance" in the error message.

Ref rust-lang/crates.io#1709
@rust-highfive

This comment has been minimized.

Copy link

commented May 13, 2019

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

This sounds like a big UX improvement. The code looks good to me. But I don't know this part of the code well enough to r+.

I wonder how hard it would be to check if the source.url is crates.io. If it is, then we can be as specific as we want.

edit:
self.host.starts_with("crates.io") or sum sutch.

@sgrif

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Good idea. I've made this specific to crates.io, and given the more detailed error message

Show resolved Hide resolved src/crates-io/lib.rs Outdated
@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

📌 Commit 4d93f2d has been approved by alexcrichton

bors added a commit that referenced this pull request May 14, 2019

Auto merge of #6936 - sgrif:sg-timeout-error, r=alexcrichton
Give a better error message when crates.io requests time out

crates.io is hosted on Heroku, which means we have a hard 30 second time
limit on all requests. Typically this is only hit when someone is
attempting to upload a crate so large that it would have been eventually
rejected anyway, but it can also happen if a user is on a very slow
internet connection.

When this happens, the request is terminated by the platform and we have
no control over the response that gets sent. This results in the user
getting a very unhelpful error message from Cargo, and some generic
error page HTML spat out into their terminal. We could work around this
on our end by adding a 29 second timeout *somewhere else* in the stack,
but we have a lot of layers that buffer requests to protect against slow
client attacks, and it'd be a pretty decent amount of work. Since we
eventually want to switch over to having Cargo do the S3 upload instead
of us, it doesn't make sense to spend so much time on an error scenario
that eventually will go away.

I've tried to keep this uncoupled from crates.io as much as possible,
since alternate registries might not be hosted on Heroku or have the
same restricitions. But I figure "a 503 that took more than 30 seconds"
is a safe bet on this being hit. If we're ok with coupling this to
crates.io, I'd like to include "If your crate is less than 10MB you can
email help@crates.io for assistance" in the error message.

Ref rust-lang/crates.io#1709
@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

⌛️ Testing commit 4d93f2d with merge 958f4a1...

@bors

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 958f4a1 to master...

@bors bors merged commit 4d93f2d into rust-lang:master May 14, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@ehuss ehuss referenced this pull request May 16, 2019

Merged

Update cargo #60874

bors added a commit to rust-lang/rust that referenced this pull request May 16, 2019

Auto merge of #60874 - ehuss:update-cargo, r=alexcrichton
Update cargo

17 commits in 759b6161a328db1d4863139e90875308ecd25a75..c4fcfb725b4be00c72eb9cf30c7d8b095577c280
2019-05-06 20:47:49 +0000 to 2019-05-15 19:48:47 +0000
- tests: registry: revert readonly permission after running tests. (rust-lang/cargo#6947)
- Remove Candidate (rust-lang/cargo#6946)
- Fix for "Running cargo update without a Cargo.lock ignores arguments" rust-lang/cargo#6872 (rust-lang/cargo#6904)
- Fix a minor mistake in the changelog. (rust-lang/cargo#6944)
- Give a better error message when crates.io requests time out (rust-lang/cargo#6936)
- Re-enable compatibility with readonly CARGO_HOME (rust-lang/cargo#6940)
- Fix version of `ignore`. (rust-lang/cargo#6938)
- Stabilize offline mode. (rust-lang/cargo#6934)
- zsh: Add doc options to include non-public items documentation (rust-lang/cargo#6929)
- zsh: Suggest --lib option as binary template now the default (rust-lang/cargo#6926)
- Migrate package include/exclude to gitignore patterns. (rust-lang/cargo#6924)
- Implement the Cargo half of pipelined compilation (take 2) (rust-lang/cargo#6883)
- Always include `Cargo.toml` when packaging. (rust-lang/cargo#6925)
- Remove unnecessary calls to masquerade_as_nightly_cargo. (rust-lang/cargo#6923)
- download: fix "Downloaded 1 crates" message (crates -> crate) (rust-lang/cargo#6920)
- Changed RUST_LOG usage to CARGO_LOG to avoid confusion. (rust-lang/cargo#6918)
- crate download: don't print that a crate was the largest download if it was the only download (rust-lang/cargo#6916)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.