Skip to content

Commit

Permalink
fix: Revert "feat: Limit HTTP concurrency and frequency by default" (#…
Browse files Browse the repository at this point in the history 
…27765)
  • Loading branch information
@rarkins
rarkins committed Mar 7, 2024
1 parent 10c8182 commit 4bfd0f3
Show file tree
Hide file tree
Showing 10 changed files with 41 additions and 166 deletions.
8 changes: 4 additions & 4 deletions lib/config/options/index.ts
View file
Expand Up @@ -2416,21 +2416,21 @@ const options: RenovateOptions[] = [
},
{
name: 'concurrentRequestLimit',
description: 'Limit concurrent requests per host. Set to 0 for no limit.',
description: 'Limit concurrent requests per host.',
type: 'integer',
default: 5,
stage: 'repository',
parents: ['hostRules'],
default: null,
cli: false,
env: false,
},
{
name: 'maxRequestsPerSecond',
description: 'Limit requests rate per host. Set to 0 for no limit.',
description: 'Limit requests rate per host.',
type: 'integer',
default: 5,
stage: 'repository',
parents: ['hostRules'],
default: 0,
cli: false,
env: false,
},
Expand Down
51 changes: 1 addition & 50 deletions lib/util/http/host-rules.spec.ts
View file
Expand Up @@ -3,12 +3,7 @@ import { bootstrap } from '../../proxy';
import type { HostRule } from '../../types';
import * as hostRules from '../host-rules';
import { dnsLookup } from './dns';
import {
applyHostRule,
findMatchingRule,
getConcurrentRequestsLimit,
getThrottleIntervalMs,
} from './host-rules';
import { applyHostRule, findMatchingRule } from './host-rules';
import type { GotOptions } from './types';

const url = 'https://github.com';
Expand Down Expand Up @@ -565,48 +560,4 @@ 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 0', () => {
hostRules.add({
matchHost: 'https://example.com',
concurrentRequestLimit: 0,
});
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 0', () => {
hostRules.add({
matchHost: 'https://example.com',
maxRequestsPerSecond: 0,
});
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: 8 additions & 28 deletions lib/util/http/host-rules.ts
View file
Expand Up @@ -217,36 +217,16 @@ export function applyHostRule<GotOptions extends HostRulesGotOptions>(
return options;
}

export function getConcurrentRequestsLimit(
url: string,
defaultValue: number | null,
): number | null {
export function getConcurrentRequestsLimit(url: string): number | null {
const { concurrentRequestLimit } = hostRules.find({ url });

if (!is.number(concurrentRequestLimit)) {
return defaultValue;
}

if (concurrentRequestLimit > 0) {
return concurrentRequestLimit;
}

return null;
return is.number(concurrentRequestLimit) && concurrentRequestLimit > 0
? concurrentRequestLimit
: null;
}

export function getThrottleIntervalMs(
url: string,
defaultValue: number | null,
): number | null {
export function getThrottleIntervalMs(url: string): number | null {
const { maxRequestsPerSecond } = hostRules.find({ url });

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;
return is.number(maxRequestsPerSecond) && maxRequestsPerSecond > 0
? Math.ceil(1000 / maxRequestsPerSecond)
: null;
}
58 changes: 12 additions & 46 deletions lib/util/http/index.spec.ts
View file
Expand Up @@ -516,68 +516,34 @@ describe('util/http/index', () => {
});

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

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

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

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

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'),
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'),
]);
await http.get('http://renovate.com/foo');
await http.get('http://renovate.com/foo');
const t2 = Date.now();

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

it('limits request rate by host', async () => {
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');
jest.useFakeTimers({ advanceTimers: true });
httpMock.scope(baseUrl).get('/foo').twice().reply(200, 'bar');
hostRules.add({ matchHost: 'renovate.com', maxRequestsPerSecond: 0.25 });

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'),
http.get('http://renovate.com/foo'),
]);
await http.get('http://renovate.com/foo');
jest.advanceTimersByTime(4000);
await http.get('http://renovate.com/foo');
const t2 = Date.now();

expect(t2 - t1).toBeWithin(800, 800 + delta);
expect(t2 - t1).toBeGreaterThanOrEqual(4000);
});
});

Expand Down
12 changes: 2 additions & 10 deletions lib/util/http/index.ts
View file
Expand Up @@ -126,14 +126,6 @@ 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;
}

private options?: GotOptions;

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

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

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

const queue = getQueue(url, Http.defaultConcurrentRequestLimit);
const queue = getQueue(url);
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
View file
Expand Up @@ -12,15 +12,15 @@ describe('util/http/queue', () => {
});

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

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

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

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

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

export function getQueue(
url: string,
defaultConcurrentRequestLimit: number | null,
): PQueue | null {
export function getQueue(url: string): PQueue | null {
const host = parseUrl(url)?.host;
if (!host) {
// should never happen
Expand All @@ -19,10 +16,7 @@ export function getQueue(
let queue = hostQueues.get(host);
if (queue === undefined) {
queue = null; // null represents "no queue", as opposed to undefined
const concurrency = getConcurrentRequestsLimit(
url,
defaultConcurrentRequestLimit,
);
const concurrency = getConcurrentRequestsLimit(url);
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
View file
Expand Up @@ -12,15 +12,15 @@ describe('util/http/throttle', () => {
});

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

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

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

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

export function getThrottle(
url: string,
defaultMaxRequestsPerSecond: number | null,
): Throttle | null {
export function getThrottle(url: string): Throttle | null {
const host = parseUrl(url)?.host;
if (!host) {
// should never happen
Expand All @@ -36,10 +33,7 @@ export function getThrottle(
let throttle = hostThrottles.get(host);
if (throttle === undefined) {
throttle = null; // null represents "no throttle", as opposed to undefined
const throttleOptions = getThrottleIntervalMs(
url,
defaultMaxRequestsPerSecond,
);
const throttleOptions = getThrottleIntervalMs(url);
if (throttleOptions) {
const intervalMs = throttleOptions;
logger.debug(`Using throttle ${intervalMs} intervalMs for host ${host}`);
Expand Down
2 changes: 0 additions & 2 deletions lib/workers/global/initialize.ts
View file
Expand Up @@ -10,7 +10,6 @@ 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 @@ -80,7 +79,6 @@ export async function globalInitialize(
config_: AllConfig,
): Promise<RenovateConfig> {
let config = config_;
Http.setDefaultLimits();
await checkVersions();
setGlobalHostRules(config);
config = await initPlatform(config);
Expand Down

0 comments on commit 4bfd0f3

Please sign in to comment.