RequestScheduler doesn't honor throttle settings
- Dominant language
- JavaScript
- Stars
- 15.7k
- Forks
- 3.9k
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 34
Description
RequestScheduler doesn't honor throttle settings and makes it easy for one server to starve all others. Take this code for example:
```js
for (var i = 0; i < RequestScheduler.priorityHeapLength; i++) {
var promise = RequestScheduler.request(new Request({
url: 'http://test.invalid/1',
throttle: true,
throttleByServer: true,
requestFunction: function() { return when.defer(); }
}));
}
```
I would expect `promise` to be undefined for all calls after `i = 6`, since 6 is the limit per-server. Unfortunately, all of them succeed. This is because the per-server numbers used by the `request` function to ensure throttling are only updated once per frame and we're instead running into the overall `priorityHeapLength` limit. The end result here is that a 3D Tiles or other provider that schedules lots of requests in one frame (not uncommon) will then starve every other provider making requests in that same frame. The only reason our unit tests pass is because they cheat by manually calling `RequestScheduler.update` whenever they need stuff to work. This obviously doesn't fly during actual use.
The "simple" fix is to update the per-server throttle value immediately on insert, but @lilleyse is worried that doing that will affect prioritization because and more thought is needed to figure out the solution.
I know I sound like a broken record, but our `RequestScheduler` continues to do more harm than good and we should seriously consider disabling it completely until we have the time to properly review and refactor it.
CC @pjcozzi @lilleyse
Contributor guide
Research direction
Start with RequestScheduler.request and RequestScheduler.update, then inspect the existing RequestScheduler unit tests and reproduce the provided loop. Determine how per-server throttle counts are updated during insertion and prioritization; done means the per-server limit is enforced during a frame without breaking request prioritization or existing tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- networking, performance
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100