Closed jbearer closed 3 months ago
ah please bump version
Offhand I'm guessing the test timeout is from the NoStorage
tests, which depend heavily on proactive fetching. We already turn the proactive fetching intervals way down for those tests. We probably also have to turn down the new delay to get them to pass.
Offhand I'm guessing the test timeout is from the
NoStorage
tests, which depend heavily on proactive fetching. We already turn the proactive fetching intervals way down for those tests. We probably also have to turn down the new delay to get them to pass.
We now have rate limiting at the low-level fetcher, which is an excellent failsafe. However, by the time we get to that point, we have already consumed resources: memory for the list of waiters on the semaphore, memory for the task doing the fetch, memory for the fetch request itself, and memory for the list of callbacks on each item which is fetched for more than one purpose.
In most cases this is fine, because we just don't spawn that many active fetches at once. However, for proactive fetching, we may request thousands of active fetches at the same time, and even though we are now limiting the network traffic they generate, we are not limiting the amount of memory they consume.
This change adds a simple
await
instead of dropping/detaching the active fetch futures so that the proactive fetching task cannot proceed faster than the fetches can be completed. Note that we still get parallelism up to the chunk size.