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

Warnings that occur when HTTP2 downloads are throttled are a little bit aggressive #316

Open
thoughtpolice opened this issue Jun 30, 2023 · 0 comments · May be fixed by #342
Open

Warnings that occur when HTTP2 downloads are throttled are a little bit aggressive #316

thoughtpolice opened this issue Jun 30, 2023 · 0 comments · May be fixed by #342

Comments

@thoughtpolice
Copy link
Contributor

thoughtpolice commented Jun 30, 2023

https://github.com/thoughtpolice/buck2/actions/runs/5418361387/jobs/9850380998

If you check the Build Buck2 with Buck2 step, you'll a duplicate number of rather aggressive annoying — I believe these originate from being too aggressive when downloading packages from https://crates.io, and getting rate limited:

WARN buck2_common::http::retries: Retrying a HTTP error after 2 seconds: HTTP Client error: HTTP: Error sending request: http2 error: stream error received: refused stream before processing any application logic

However, it's actually a bit worse when superconsole is in use in your terminal — here it just uses a more typical log stream that streams once — because it gets spammed a bunch during refresh of the active targets being built. Looks like it happens dozens of times in Windows Terminal for me, at least. I can attach a small asciinema of this but it should be easy to reproduce, I think.

Honestly, I don't think this warning as it stands now is really that important, a little rate limiting is expected if you're doing a fresh build with no cache, I think. You're downloading like 100+ dependent crates pretty quickly in this scenario. The download phase of the crates being extended by a second or two is probably OK if you end up getting a big set of them in batches, it may only add a few seconds to the initial download actions.

I think better logic would be if Buck2:

  • Downloads with typical exponential backoff during throttling (I assume this is the case already), and
  • Only throws an error after a number of retries results in, say, a backoff window of 60 seconds or more occurs.

60 seconds is arbitrary but seems like it's egregious enough to make that a big orange warning. A hair you'd have to split is whether or not you would only do one warning or just warn on every single action. Currently it looks like the warning occurs for every action that gets throttled, but maybe you only want one warning per HTTP domain; e.g. if two download_file actions to download two separate files from http://foo.example.com get rate limited, only report once for the foo.example.com domain — and if another download_file call to http://bar.example.com also gets rate limited, report that, so you'd only have two warnings total.

thoughtpolice added a commit to thoughtpolice/buck2 that referenced this issue Jul 1, 2023
Summary: When downloading from sites like crates.io, we can pretty easily get
rate-limited when a lot of our crates are uncached. Don't be so aggressive about
warning; most small backoffs won't be too noticeable.

As a note, we _don't_ emit the error message we get from the callstack here in
the message; the logic is that it's already a retriable error, so it doesn't
make much sense to spam an error that we'd probably get again anyway, and
that we ultimately are going to re-attempt. This should just make the UX a
little more polished.

Also emit the domain name we're hitting problems with.

Test Plan: Change the new warning conditional from 30 seconds to zero seconds.
Build buck2 with buck2. Observe the new error message, which helpfully points
out we're connecting to crates.io. Change the conditional back to 30 seconds.
Rebuild buck2 with buck2. Note that no warnings appear, because the exponential
backoff only results in a few small 2-second retries.

GitHub Issue: Fixes facebook#316

Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: Ixoxuusxlkkrspllqqvukzrumnrytmpko
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant