respect limit when throttle is also set - #503
Open
rootkiller6788 wants to merge 2 commits into
Open
rootkiller6788 wants to merge 2 commits into
rootkiller6788 wants to merge 2 commits into
Conversation
The limit was only checked against `toPrefetch.size` at the moment a link was scheduled, but that set only grows once a task actually runs. With a throttle set, tasks pile up in the throttler queue, the set stays where it is, and every remaining link in the same observer callback passes the check and gets queued. Setting limit: 4 with throttle: 3 prefetched all 20 links in the page from issue GoogleChromeLabs#235. Count the tasks that are queued but not started yet and reserve a slot for them at scheduling time. The unthrottled path is unaffected: the task body runs synchronously, so the count is back to zero before the next link is looked at.
4 links in view, limit: 2, throttle: 1. All four land in the same observer callback, so the queue keeps the second half from starting until the first half is done. Before the fix this prefetched all 4 links. I used limit 2 / throttle 1 instead of the 4 / 3 from the issue because it reproduces the same thing with fewer moving parts, and the suite already has a test-throttle fixture that shows the pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Set
limit: 4andthrottle: 3on a page with 20 links and all 20 got prefetched, same as in the issue.The limit is checked when a link is scheduled, but
toPrefetchonly grows once a throttled task actually starts. With a throttle set the tasks sit in the queue, the set stays put, and every remaining link in the same observer callback passes the check and gets queued too. That is why it only shows up when limit is larger than throttle.So I count the tasks that have been handed to the throttler but haven't started yet, and reserve a slot for them at scheduling time. Without a throttle nothing changes - the task body runs synchronously, so the count is back to zero before the next link is looked at.
The test is a 4 link page with
limit: 2, throttle: 1. All four links land in the same observer callback, so the second pair has to wait for the first pair to finish. 4 requests before, 2 now. Full suite is green (20/20).I left
src/chunks.mjsalone - it has the same check and the same problem, but the react-chunks build has no test coverage here, so I didn't want to change it without being able to run something.