diff --git a/source/index.ts b/source/index.ts index 7bcbb0d..a5730fa 100644 --- a/source/index.ts +++ b/source/index.ts @@ -1323,6 +1323,33 @@ export function getCredentialedURL( return url } +/** + * The concurrency used when {@link QueryOptions.concurrency} is not specified. + * GitHub allows no more than 100 concurrent requests, shared across its REST and GraphQL APIs. + * https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits + */ +export const defaultConcurrency = 100 + +/** + * The pools shared by all queries, keyed by their concurrency. + * Shared because query options are spread into a new object on their way to + * {@link queryREST} and {@link queryGraphQL}, so a pool created against those + * options would only ever apply to the single request that created it. + */ +const pools = new Map>() + +/** Get the pool shared by all queries of this concurrency, creating it if needed. */ +export function getPool( + concurrency: number = defaultConcurrency, +): PromisePool { + let pool = pools.get(concurrency) + if (!pool) { + pool = new PromisePool(concurrency) + pools.set(concurrency, pool) + } + return pool +} + /** * Fetches a GitHub REST API response with authentication, parsing, waiting, pooling, paging. * If the credentials property is nullish, then the environment variables are attempted. @@ -1330,7 +1357,7 @@ export function getCredentialedURL( */ export async function queryREST(opts: QueryOptions = {}): Promise { // defaults - opts.pool ??= new PromisePool(opts.concurrency) + opts.pool ??= getPool(opts.concurrency) const searchParams = new URLSearchParams() applySearchParams(searchParams, opts.searchParams) if (opts.page != null || opts.pages != null || opts.size != null) { @@ -1438,7 +1465,7 @@ export async function queryGraphQL( opts: QueryOptions = {}, ): Promise { // prepare - opts.pool ??= new PromisePool(opts.concurrency) + opts.pool ??= getPool(opts.concurrency) // prepare fetch // https://docs.github.com/en/graphql/overview/explorer diff --git a/source/test.ts b/source/test.ts index 08eb1bf..364dc6b 100644 --- a/source/test.ts +++ b/source/test.ts @@ -26,6 +26,9 @@ import getGitHubLatestCommit, { getGitHubRepositoriesFromSearch, getGitHubSlugFromUrl, hasCredentials, + getPool, + defaultConcurrency, + QueryOptions, } from './index.js' type Errback = (error?: Error) => void @@ -243,6 +246,79 @@ kava.suite('@bevry/github-api', function (suite, test) { suite('redact', function (suite, test) { testFixtures(redactFixtures, redactSearchParams, test) }) + suite('pool', function (suite, test) { + test('limits concurrent requests', function (done: Errback) { + // Drives the public path with a stubbed fetch, so it needs no network + // and no credentials. Before the pool was shared, this peaked at the + // full request count rather than the requested concurrency. + const originalFetch = globalThis.fetch + const concurrency = 3 + const requests = 30 + let inflight = 0 + let peak = 0 + globalThis.fetch = async function () { + inflight++ + peak = Math.max(peak, inflight) + await new Promise((resolve) => setTimeout(resolve, 15)) + inflight-- + return { + ok: true, + status: 200, + statusText: 'OK', + json: async () => ({ ok: true }), + } + } as any + const credentials: GitHubCredentials = { GITHUB_ACCESS_TOKEN: 'stub' } + Promise.all( + Array.from({ length: requests }, () => + queryREST({ pathname: 'rate_limit', concurrency, credentials }), + ), + ) + .finally(() => { + globalThis.fetch = originalFetch + }) + .then(() => { + equal( + peak <= concurrency, + true, + `peak of ${peak} concurrent requests was within the concurrency of ${concurrency}`, + ) + done() + }) + .catch(done) + }) + test('shares one pool across spread options', function () { + // The bug this guards against: queryREST and queryGraphQL receive + // `{ ...opts }` from their callers, so a pool constructed from those + // options applied only to the single request that constructed it, and + // the concurrency option silently limited nothing. + const opts: QueryOptions = { concurrency: 5 } + const a: QueryOptions = { ...opts } + const b: QueryOptions = { ...opts } + a.pool ??= getPool(a.concurrency) + b.pool ??= getPool(b.concurrency) + equal(a.pool === b.pool, true, 'spread options resolved to the same pool') + }) + test('separates pools by concurrency', function () { + equal( + getPool(5) === getPool(7), + false, + 'a different concurrency resolved to a different pool', + ) + }) + test('defaults to defaultConcurrency', function () { + equal( + getPool() === getPool(defaultConcurrency), + true, + 'an unspecified concurrency resolved to the default pool', + ) + equal( + getPool().concurrency, + defaultConcurrency, + 'the default pool limits to the documented concurrency', + ) + }) + }) suite('api', function (suite, test) { if (!hasCredentials()) { console.warn('unable to test API, as github credentials not set')