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

feat: Limit HTTP concurrency and frequency by default #27621

Merged
merged 19 commits into from Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/config/options/index.ts
Expand Up @@ -2416,17 +2416,18 @@ const options: RenovateOptions[] = [
},
{
name: 'concurrentRequestLimit',
description: 'Limit concurrent requests per host.',
description:
'Limit concurrent requests per host. Set to `-1` for no limit. Default value is 5 concurrent requests.',
zharinov marked this conversation as resolved.
Show resolved Hide resolved
type: 'integer',
stage: 'repository',
parents: ['hostRules'],
default: null,
rarkins marked this conversation as resolved.
Show resolved Hide resolved
cli: false,
env: false,
},
{
name: 'maxRequestsPerSecond',
description: 'Limit requests rate per host.',
description:
'Limit requests rate per host. Set to `-1` for no limit. Default value is 5 requests per second.',
zharinov marked this conversation as resolved.
Show resolved Hide resolved
zharinov marked this conversation as resolved.
Show resolved Hide resolved
type: 'integer',
stage: 'repository',
parents: ['hostRules'],
Expand Down
51 changes: 50 additions & 1 deletion lib/util/http/host-rules.spec.ts
Expand Up @@ -3,7 +3,12 @@ import { bootstrap } from '../../proxy';
import type { HostRule } from '../../types';
import * as hostRules from '../host-rules';
import { dnsLookup } from './dns';
import { applyHostRule, findMatchingRule } from './host-rules';
import {
applyHostRule,
findMatchingRule,
getConcurrentRequestsLimit,
getThrottleIntervalMs,
} from './host-rules';
import type { GotOptions } from './types';

const url = 'https://github.com';
Expand Down Expand Up @@ -560,4 +565,48 @@ describe('util/http/host-rules', () => {
},
});
});

describe('getConcurrentRequestsLimit', () => {
it('returns default value for undefined rule', () => {
expect(getConcurrentRequestsLimit('https://example.com', 42)).toBe(42);
});

it('returns null for -1', () => {
hostRules.add({
matchHost: 'https://example.com',
concurrentRequestLimit: -1,
});
expect(getConcurrentRequestsLimit('https://example.com', 42)).toBeNull();
});

it('returns positive limit', () => {
hostRules.add({
matchHost: 'https://example.com',
concurrentRequestLimit: 143,
});
expect(getConcurrentRequestsLimit('https://example.com', 42)).toBe(143);
});
});

describe('getThrottleIntervalMs', () => {
it('returns default value for undefined rule', () => {
expect(getThrottleIntervalMs('https://example.com', 42)).toBe(24); // 1000 / 42
});

it('returns null for -1', () => {
hostRules.add({
matchHost: 'https://example.com',
maxRequestsPerSecond: -1,
});
expect(getThrottleIntervalMs('https://example.com', 42)).toBeNull();
});

it('returns positive limit', () => {
hostRules.add({
matchHost: 'https://example.com',
maxRequestsPerSecond: 143,
});
expect(getThrottleIntervalMs('https://example.com', 42)).toBe(7); // 1000 / 143
});
});
});
36 changes: 28 additions & 8 deletions lib/util/http/host-rules.ts
Expand Up @@ -217,16 +217,36 @@ export function applyHostRule<GotOptions extends HostRulesGotOptions>(
return options;
}

export function getConcurrentRequestsLimit(url: string): number | null {
export function getConcurrentRequestsLimit(
url: string,
defaultValue: number | null,
): number | null {
const { concurrentRequestLimit } = hostRules.find({ url });
return is.number(concurrentRequestLimit) && concurrentRequestLimit > 0
? concurrentRequestLimit
: null;

if (!is.number(concurrentRequestLimit)) {
zharinov marked this conversation as resolved.
Show resolved Hide resolved
return defaultValue;
}

if (concurrentRequestLimit > 0) {
return concurrentRequestLimit;
}

return null;
}

export function getThrottleIntervalMs(url: string): number | null {
export function getThrottleIntervalMs(
url: string,
defaultValue: number | null,
): number | null {
const { maxRequestsPerSecond } = hostRules.find({ url });
return is.number(maxRequestsPerSecond) && maxRequestsPerSecond > 0
? Math.ceil(1000 / maxRequestsPerSecond)
: null;

if (!is.number(maxRequestsPerSecond)) {
return defaultValue ? Math.ceil(1000 / defaultValue) : defaultValue; // 5 requests per second
}

if (maxRequestsPerSecond > 0) {
return Math.ceil(1000 / maxRequestsPerSecond);
}

return null;
}
58 changes: 46 additions & 12 deletions lib/util/http/index.spec.ts
Expand Up @@ -516,34 +516,68 @@ describe('util/http/index', () => {
});

describe('Throttling', () => {
const delta = 100;

beforeEach(() => {
jest.useFakeTimers({ advanceTimers: true });
});

afterEach(() => {
jest.useRealTimers();
});

it('works without throttling', async () => {
jest.useFakeTimers({ advanceTimers: 1 });
httpMock.scope(baseUrl).get('/foo').twice().reply(200, 'bar');
httpMock.scope(baseUrl).get('/foo').times(10).reply(200, 'bar');

const t1 = Date.now();
await http.get('http://renovate.com/foo');
await http.get('http://renovate.com/foo');
await Promise.all([
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
]);
const t2 = Date.now();

expect(t2 - t1).toBeLessThan(100);
expect(t2 - t1).toBeLessThan(delta);
});

it('limits request rate by host', async () => {
jest.useFakeTimers({ advanceTimers: true });
httpMock.scope(baseUrl).get('/foo').twice().reply(200, 'bar');
hostRules.add({ matchHost: 'renovate.com', maxRequestsPerSecond: 0.25 });
httpMock.scope(baseUrl).get('/foo').times(4).reply(200, 'bar');
hostRules.add({ matchHost: 'renovate.com', maxRequestsPerSecond: 3 });

const t1 = Date.now();
await Promise.all([
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
]);
const t2 = Date.now();

expect(t2 - t1).toBeWithin(1000, 1000 + delta);
});

it('defaults to 5 requests per second', async () => {
Http.setDefaultLimits();
httpMock.scope(baseUrl).get('/foo').times(5).reply(200, 'bar');

const t1 = Date.now();
await http.get('http://renovate.com/foo');
jest.advanceTimersByTime(4000);
await http.get('http://renovate.com/foo');
await Promise.all([
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
http.get('http://renovate.com/foo'),
]);
const t2 = Date.now();

expect(t2 - t1).toBeGreaterThanOrEqual(4000);
expect(t2 - t1).toBeWithin(800, 800 + delta);
rarkins marked this conversation as resolved.
Show resolved Hide resolved
});
});

Expand Down
12 changes: 10 additions & 2 deletions lib/util/http/index.ts
Expand Up @@ -126,6 +126,14 @@ async function gotTask<T>(
}

export class Http<Opts extends HttpOptions = HttpOptions> {
private static defaultConcurrentRequestLimit: number | null = null;
private static defaultMaxRequestsPerSecond: number | null = null;

static setDefaultLimits(): void {
Http.defaultConcurrentRequestLimit = 5;
Http.defaultMaxRequestsPerSecond = 5;
}

rarkins marked this conversation as resolved.
Show resolved Hide resolved
private options?: GotOptions;

constructor(
Expand All @@ -143,7 +151,7 @@ export class Http<Opts extends HttpOptions = HttpOptions> {
}

protected getThrottle(url: string): Throttle | null {
return getThrottle(url);
return getThrottle(url, Http.defaultMaxRequestsPerSecond);
}

protected async request<T>(
Expand Down Expand Up @@ -249,7 +257,7 @@ export class Http<Opts extends HttpOptions = HttpOptions> {
? () => throttle.add<HttpResponse<T>>(httpTask)
: httpTask;

const queue = getQueue(url);
const queue = getQueue(url, Http.defaultConcurrentRequestLimit);
const queuedTask: GotTask<T> = queue
? () => queue.add<HttpResponse<T>>(throttledTask)
: throttledTask;
Expand Down
10 changes: 5 additions & 5 deletions lib/util/http/queue.spec.ts
Expand Up @@ -12,15 +12,15 @@ describe('util/http/queue', () => {
});

it('returns null for invalid URL', () => {
expect(getQueue('$#@!')).toBeNull();
expect(getQueue('$#@!', null)).toBeNull();
});

it('returns queue for valid url', () => {
const q1a = getQueue('https://example.com');
const q1b = getQueue('https://example.com');
const q1a = getQueue('https://example.com', null);
const q1b = getQueue('https://example.com', null);

const q2a = getQueue('https://example.com:8080');
const q2b = getQueue('https://example.com:8080');
const q2a = getQueue('https://example.com:8080', null);
const q2b = getQueue('https://example.com:8080', null);

expect(q1a).not.toBeNull();
expect(q1a).toBe(q1b);
Expand Down
10 changes: 8 additions & 2 deletions lib/util/http/queue.ts
Expand Up @@ -5,7 +5,10 @@ import { getConcurrentRequestsLimit } from './host-rules';

const hostQueues = new Map<string, PQueue | null>();

export function getQueue(url: string): PQueue | null {
export function getQueue(
url: string,
defaultConcurrentRequestLimit: number | null,
): PQueue | null {
const host = parseUrl(url)?.host;
if (!host) {
// should never happen
Expand All @@ -16,7 +19,10 @@ export function getQueue(url: string): PQueue | null {
let queue = hostQueues.get(host);
if (queue === undefined) {
queue = null; // null represents "no queue", as opposed to undefined
const concurrency = getConcurrentRequestsLimit(url);
const concurrency = getConcurrentRequestsLimit(
url,
defaultConcurrentRequestLimit,
);
if (concurrency) {
logger.debug(`Using queue: host=${host}, concurrency=${concurrency}`);
queue = new PQueue({ concurrency });
Expand Down
10 changes: 5 additions & 5 deletions lib/util/http/throttle.spec.ts
Expand Up @@ -12,15 +12,15 @@ describe('util/http/throttle', () => {
});

it('returns null for invalid URL', () => {
expect(getThrottle('$#@!')).toBeNull();
expect(getThrottle('$#@!', null)).toBeNull();
});

it('returns throttle for valid url', () => {
const t1a = getThrottle('https://example.com');
const t1b = getThrottle('https://example.com');
const t1a = getThrottle('https://example.com', null);
const t1b = getThrottle('https://example.com', null);

const t2a = getThrottle('https://example.com:8080');
const t2b = getThrottle('https://example.com:8080');
const t2a = getThrottle('https://example.com:8080', null);
const t2b = getThrottle('https://example.com:8080', null);

expect(t1a).not.toBeNull();
expect(t1a).toBe(t1b);
Expand Down
10 changes: 8 additions & 2 deletions lib/util/http/throttle.ts
Expand Up @@ -22,7 +22,10 @@ export class Throttle {
}
}

export function getThrottle(url: string): Throttle | null {
export function getThrottle(
url: string,
defaultMaxRequestsPerSecond: number | null,
): Throttle | null {
const host = parseUrl(url)?.host;
if (!host) {
// should never happen
Expand All @@ -33,7 +36,10 @@ export function getThrottle(url: string): Throttle | null {
let throttle = hostThrottles.get(host);
if (throttle === undefined) {
throttle = null; // null represents "no throttle", as opposed to undefined
const throttleOptions = getThrottleIntervalMs(url);
const throttleOptions = getThrottleIntervalMs(
url,
defaultMaxRequestsPerSecond,
);
if (throttleOptions) {
const intervalMs = throttleOptions;
logger.debug(`Using throttle ${intervalMs} intervalMs for host ${host}`);
Expand Down
2 changes: 2 additions & 0 deletions lib/workers/global/initialize.ts
Expand Up @@ -10,6 +10,7 @@ import * as packageCache from '../../util/cache/package';
import { setEmojiConfig } from '../../util/emoji';
import { validateGitVersion } from '../../util/git';
import * as hostRules from '../../util/host-rules';
import { Http } from '../../util/http';
import { initMergeConfidence } from '../../util/merge-confidence';
import { setMaxLimit } from './limits';

Expand Down Expand Up @@ -79,6 +80,7 @@ export async function globalInitialize(
config_: AllConfig,
): Promise<RenovateConfig> {
let config = config_;
Http.setDefaultLimits();
await checkVersions();
setGlobalHostRules(config);
config = await initPlatform(config);
Expand Down