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

Improve Crates.io rate limiting response display #9119

Closed
wants to merge 2 commits into from

Conversation

CPerezz
Copy link
Contributor

@CPerezz CPerezz commented Jan 31, 2021

In rust-lang/crates.io#1596 it was added
a rate limit for crate publishing endpoint connections (10 components/min).
As is said in the PR. The strategy used allows to upload 10 components
first and then the ratio starts to be applied.
Now, if the rate is surpassed, crates.io returns a 429 HTTP response.

Cargo, tries to publish all of the workspace/crate components as fast
as possible (as we should expect) but when we have more than 10
components and we try to publish the 11th, usually a minute hasn't
passed yet and therefore we get a HTTP-429.

This made some users to experience confusing/not well explained error
messages as seen in rust-lang/crates.io#1643.

The limit got increased to 30 components/min. But who knows..

So this closes #6714 while
a better solution is not found.

In rust-lang/crates.io#1596 it was added
a rate limit for crate publishing endpoint connections (10 components/min).
As is said in the PR. The strategy used allows to upload 10 components
first and then the ratio starts to be applied.
Now, if the rate is surpassed, crates.io returns a 429 HTTP response.

Cargo, tries to publish all of the workspace/crate components as fast
as possible (as we should expect) but when we have more than 10
components and we try to publish the 11th, usually a minute hasn't
passed yet and therefore we get a HTTP-429.

This made some users to experience confusing/not well explained error
messages as seen in rust-lang/crates.io#1643.

The limit got increased to 30 components/min. But who knows..

So this closes rust-lang#6714 while
a better solution is not found.
@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 31, 2021

I don't know if hard coding this error message is a good plan. For one thing what happens if a user is publishing to an alternative registry? ( But the next message is also only for crates.io. So maybe I don't have all the context.) For another thing crates.io may change its policy, and so this won't be correct.

Is there context I am missing? Or, can we make a more generic message? Or, can we use some data from the response?

@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 1, 2021

Hi @Eh2406 thanks for the feedback.

Is there context I am missing?

In respect of the context I think @carols10cents is the one who ask for. She'll know much better than me (I just tried to fix the issue and read across some of the issues and PRs related.

For one thing what happens if a user is publishing to an alternative registry?

You're right on the crates.io check and the alternative registry. Added self.host_is_crates_io() check for it.

For another thing crates.io may change its policy, and so this won't be correct

In order to be tight to their policy, and be able to respond to any changes, my suggestion is to import cargo-registry and import:
https://github.com/rust-lang/crates.io/blob/master/src/publish_rate_limit.rs#L9-L22
When calling Default for it we can get the burst allowed and print it in the message. So it always gets updated with path and minor updates of the lib. And then we're always in sync with them. Is anything is removed or changed, we will notice since a new major version will be out.

Or, can we make a more generic message?

The error is quite specific indeed. 429 isn't a typical HTTP response code, so I don't see how we can make the error more generic.

Or, can we use some data from the response?

The response does not provide any useful info AFAIK as you can see in rust-lang/crates.io#1643

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 1, 2021

my suggestion is to import cargo-registry and import

even if cargo-registry was being published regularly, that would make the error message up to date with when Cargo was built but what we need is the policy when Publish is run. If someone has an MSRV that means that they are using a Cargo that is from 2 years ago, they need todays policy not 2 years ago.

429 isn't a typical HTTP response code, so I don't see how we can make the error more generic.

Maybe we can get crates.io to include your text in the body of the message, or some metadata in the headers. If one day Cargo is going to respond to 429 by retrying, then we will need a Retry-After header to tell us how long to weight.

According to MDN it is for "Too Many Requests". So a message like "Looks like you are making too many requests of the registry. Please slow down to give the registry servers time to process. Check your registries documentation for what rates are acceptable." and if it is crates.io add " crates.io has its relevant documentation at ___" may work.

@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 1, 2021

Hi @Eh2406. Thanks a lot for the suggestions.

About the import you're completely right.

Maybe we can get crates.io to include your text in the body of the message, or some metadata in the headers. If one day Cargo is going to respond to 429 by retrying, then we will need a Retry-After header to tell us how long to weight.

According to MDN it is for "Too Many Requests". So a message like "Looks like you are making too many requests of the registry. Please slow down to give the registry servers time to process. Check your registries documentation for what rates are acceptable." and if it is crates.io add " crates.io has its relevant documentation at ___" may work.

That's true, and indeed good idea. Since Cargo (at least now) publishes as fast as possible, we can include this generic error message here and attach the crates.io one if at some point they implement one (I'll try to do so).

In the relevant documentation section, what would you add? Since there's no mention to the maximum rate anywhere aside from the issues. So I guess that the crates.io GH repo link? Since the Cargo book doesn't have much stuff about it.

Thanks!

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 1, 2021

It would be great if they could document it someplace better then the issues. https://crates.io/policies feels like a good place, we could start with a PR to change that page as its source is just here I think.

@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 1, 2021

Filed rust-lang/crates.io#3229. Once it's closed and we know which link can we refer to, I think we're good to go and address your suggestions 😃

@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 2, 2021

As it was said in rust-lang/crates.io#3229 we can close this PR since the error is being returned from crates.io now in the headers and we already display them.

So closing this. @Eh2406 I think that as you said, we can close #9119 .

@CPerezz CPerezz closed this Feb 2, 2021
@CPerezz CPerezz deleted the 429_publish_err branch February 2, 2021 08:22
@Eh2406
Copy link
Contributor

Eh2406 commented Feb 2, 2021

Thank you for all the work tracking down all the implications. While this PR did not get merged, your contribution did close a lot of issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crates.io rate limiting response display isn't ideal
3 participants