Limit concurrent HTTP requests

[Ralith]

When building a non-trivial project with third-party dependencies fetched via http_archive or similar, e.g. from reindeer in non-vendored mode, buck2 can generate extremely large numbers of outgoing HTTP requests. Hyper does not enforce any limits itself, happily expanding its connection pool with every additional concurrent request. This can cause the remote HTTP server to reject requests, and even lead to buck2 itself failing with "too many open files" errors.

Larger numbers of concurrent requests have rapidly diminishing returns, so while there's no obviously correct limit, any smallish number should improve behavior in most cases. It may even improve total throughput by reducing contention for network bandwidth.

See also discussion in facebookincubator/reindeer#46.

[facebook-github-bot]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

[Ralith]

cc @cormacrelf

[Ralith]

Reworked this to reduce the boilerplate and ensure the semaphore permit is held until the response is fully consumed. Hopefully that doesn't confuse the import?

[Ralith]

This is a little cheeky and might be clearer as a stream transformer struct, but that would probably take about 4x as much code. Let me know if that'd be preferred.

[iguridi]

why do we need this in the stream itself? Could we have this in line 142 let _guard = &semaphore_guard;. As far as I can tell the request already occurred by then

[Ralith]

These requests are typically downloading potentially large files. We don't want to release semaphore credit until the response has been completed, which is after the stream has been consumed in full and dropped. Without stream ownership of the guard, it would be released as soon as the response begins, and we could still end up with arbitrarily large numbers of concurrent [attempted] requests, which is what this PR seeks to avoid.

[Ralith]

Another thought: the motivating case for this PR is downloading from crates.io. In theory, all such requests could be pipelined through a single HTTP/2 connection, which would avoid the resource exhaustion hazard and probably be more efficient for everyone involved. Getting that to happen seems like a significantly more subtle task, however, so maybe best left for follow-up work.

[Ralith]

As described in #316 (comment) I think there's a more direct solution available by tweaking hyper's HTTP client to reduce the initial default quantity of multiplexing. I'll pursue that in a separate PR. Leaving this open for now in case there's independent interest in limiting the number of connections, though as currently written this limits the number of requests, which isn't quite right.

[cormacrelf]

A few issues:

1. Doesn't hyper just open a second HTTP/2 connection if you hit the limit of # streams per conn? This is just an http2 optimization to pack more requests into the same connection and amortise handshakes, isn't there a separate connection pool behaviour globally that is still unlimited?
2. In any case OS-level resource limits on file descriptors don't care about distinct hostnames or http2 features. This all falls down if your web server does http1 only and cannot multiplex any streams, or if you make requests to too many different hostnames. Hyper needs to be configured to enforce global limits. I can't actually find anything in hyper that limits the total size of the connection pool.
3. If hyper can in fact be configured to limit concurrency globally, you still need to ensure that the w+ files that get created for each download are not created while hyper is queuing a request. These lines in this order will still hit file descriptor limits if relying on hyper to limit concurrency, because the file is created before we start waiting for hyper to (theoretically?) finish waiting for a slot internally.

All things considered, we should have that http2 max concurrent streams change, but for file descriptor limits, the semaphore here is the only thing that will work.

[Ralith]

Thanks for the feedback!

Doesn't hyper just open a second HTTP/2 connection if you hit the limit of # streams per conn?

I'd be surprised, but I'll verify.

This all falls down if your web server does http1 only and cannot multiplex any streams, or if you make requests to too many different hostnames.

Yes, this PR is still necessary for that case.

These lines in this order will still hit file descriptor limits if relying on hyper to limit concurrency, because the file is created before we start waiting for hyper to (theoretically?) finish waiting for a slot internally.

This seems like a pretty easy fix, though? I'll draft a separate PR.

[Ralith]

Opened #991 to delay file creation. I think we need that to get the most benefit from this PR anyway, since the semaphore acquire happens when the HTTP request is attempted, after the file has already been opened otherwise.

[Ralith]

Doesn't hyper just open a second HTTP/2 connection if you hit the limit of # streams per conn?

hyper maintainers confirm that this is not the case.

[facebook-github-bot]

@iguridi merged this pull request in ef396e8.