diff --git a/.changeset/skip-clone-no-dedup.md b/.changeset/skip-clone-no-dedup.md new file mode 100644 index 00000000..5cb7b405 --- /dev/null +++ b/.changeset/skip-clone-no-dedup.md @@ -0,0 +1,5 @@ +--- +'@apollo/datasource-rest': patch +--- + +Add `shouldCloneParsedBodyForDeduplication()` so subclasses can skip per-caller cloning under high-fanout request deduplication. The default still clones concurrent consumers for isolation, and still skips the clone when a request-lifetime GET has only one consumer. diff --git a/cspell-dict.txt b/cspell-dict.txt index 255b4b24..72e69464 100644 --- a/cspell-dict.txt +++ b/cspell-dict.txt @@ -14,6 +14,7 @@ datasource direnv Fakeable falsey +fanout httpcache instanceof isplainobject diff --git a/src/RESTDataSource.ts b/src/RESTDataSource.ts index 9246f541..cf9a8315 100644 --- a/src/RESTDataSource.ts +++ b/src/RESTDataSource.ts @@ -195,6 +195,10 @@ export type RequestDeduplicationPolicy = export abstract class RESTDataSource { protected httpCache: HTTPCache; protected deduplicationPromises = new Map>(); + private deduplicationConsumers = new WeakMap< + Promise, + { count: number } + >(); baseURL?: string; logger: Logger; @@ -324,11 +328,14 @@ export abstract class RESTDataSource { 'requestDeduplication' >, requestDeduplicationResult: RequestDeduplicationResult, + cloneParsedBody = true, ): DataSourceFetchResult { return { ...dataSourceFetchResult, requestDeduplication: requestDeduplicationResult, - parsedBody: this.cloneParsedBody(dataSourceFetchResult.parsedBody), + parsedBody: cloneParsedBody + ? this.cloneParsedBody(dataSourceFetchResult.parsedBody) + : dataSourceFetchResult.parsedBody, }; } @@ -337,6 +344,27 @@ export abstract class RESTDataSource { return cloneDeep(parsedBody); } + /** + * Whether a caller of a deduplicated request should receive an independent + * clone of the parsed body. + * + * The default is `true`, which preserves isolation: mutating one caller's + * `parsedBody` does not affect other callers sharing the same HTTP request. + * + * Under high fan-out (many resolvers sharing one GET), that isolation costs + * one deep clone per consumer. Override and return `false` when callers treat + * the body as immutable to eliminate those clones. The default still skips + * cloning when `deduplicate-during-request-lifetime` has only a single + * consumer, because there is then nothing to isolate. + * + * Overriding `cloneParsedBody` to return its argument also skips these copies; + * this hook is the more explicit opt-out for "do not isolate deduplicated + * results." + */ + protected shouldCloneParsedBodyForDeduplication(): boolean { + return true; + } + protected shouldJSONSerializeBody( body: RequestWithBody['body'], ): boolean { @@ -583,19 +611,33 @@ export abstract class RESTDataSource { const previousRequestPromise = this.deduplicationPromises.get( policy.deduplicationKey, ); - if (previousRequestPromise) - return previousRequestPromise.then((result) => - this.cloneDataSourceFetchResult(result, { + if (previousRequestPromise) { + const previousConsumers = this.deduplicationConsumers.get( + previousRequestPromise, + ); + if (previousConsumers) { + previousConsumers.count += 1; + } + return previousRequestPromise.then((result) => { + const requestDeduplicationResult: RequestDeduplicationResult = { policy, deduplicatedAgainstPreviousRequest: true, - }), - ); + }; + return this.cloneDataSourceFetchResult( + result, + requestDeduplicationResult, + this.shouldCloneParsedBodyForDeduplication(), + ); + }); + } const thisRequestPromise = performRequest(); this.deduplicationPromises.set( policy.deduplicationKey, thisRequestPromise, ); + const consumers = { count: 1 }; + this.deduplicationConsumers.set(thisRequestPromise, consumers); try { // The request promise needs to be awaited here rather than just // returned. This ensures that the request completes before it's removed @@ -603,13 +645,29 @@ export abstract class RESTDataSource { // deduplication cache is cleared in the event of an error during the // request. // - // Note: we could try to get fancy and only clone if no de-duplication - // happened (and we're "deduplicate-during-request-lifetime") but we - // haven't quite bothered yet. - return this.cloneDataSourceFetchResult(await thisRequestPromise, { + // Skip cloning when this is the only consumer and the result will not + // stay cached (`deduplicate-during-request-lifetime`). Concurrent + // waiters and `deduplicate-until-invalidated` still need a clone so + // callers can mutate independently. + const result = await thisRequestPromise; + const requestDeduplicationResult: RequestDeduplicationResult = { policy, deduplicatedAgainstPreviousRequest: false, - }); + }; + // Concurrent waiters and `deduplicate-until-invalidated` need isolation + // by default. A sole `deduplicate-during-request-lifetime` consumer + // does not. Subclasses can skip remaining clones via + // `shouldCloneParsedBodyForDeduplication`. + const needsIsolation = + consumers.count > 1 || + policy.policy === 'deduplicate-until-invalidated'; + const shouldCloneParsedBody = + needsIsolation && this.shouldCloneParsedBodyForDeduplication(); + return this.cloneDataSourceFetchResult( + result, + requestDeduplicationResult, + shouldCloneParsedBody, + ); } finally { if (policy.policy === 'deduplicate-during-request-lifetime') { this.deduplicationPromises.delete(policy.deduplicationKey); diff --git a/src/__tests__/RESTDataSource.test.ts b/src/__tests__/RESTDataSource.test.ts index f3e7cda5..4b11b45b 100644 --- a/src/__tests__/RESTDataSource.test.ts +++ b/src/__tests__/RESTDataSource.test.ts @@ -1105,6 +1105,189 @@ describe('RESTDataSource', () => { `); }); + it('does not clone a request-lifetime result with a single consumer', async () => { + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + nock(apiUrl).get('/foo/1').reply(200, { hi: 43 }); + + await dataSource.getFoo(1); + await dataSource.getFoo(1); + expect(cloneCount).toEqual(0); + }); + + it('clones parsed body for each concurrent consumer', async () => { + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + + await Promise.all([dataSource.getFoo(1), dataSource.getFoo(1)]); + expect(cloneCount).toEqual(2); + }); + + it('clones parsed body once per concurrent consumer under high fan-out', async () => { + const consumerCount = 100; + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + + await Promise.all( + Array.from({ length: consumerCount }, () => dataSource.getFoo(1)), + ); + expect(cloneCount).toEqual(consumerCount); + }); + + it('does not clone under high fan-out when shouldCloneParsedBodyForDeduplication is false', async () => { + const consumerCount = 100; + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + + protected override shouldCloneParsedBodyForDeduplication(): boolean { + return false; + } + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch<{ hi: number }>(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + + const results = await Promise.all( + Array.from({ length: consumerCount }, () => dataSource.getFoo(1)), + ); + expect(cloneCount).toEqual(0); + expect(results).toHaveLength(consumerCount); + for (const result of results) { + expect(result.parsedBody).toEqual({ hi: 42 }); + expect(result.parsedBody).toBe(results[0].parsedBody); + } + }); + + it('clones parsed body for deduplicate-until-invalidated so later callers are isolated', async () => { + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + protected override requestDeduplicationPolicyFor( + url: URL, + request: WithRequired, + ): RequestDeduplicationPolicy { + const p = super.requestDeduplicationPolicyFor(url, request); + return p.policy === 'deduplicate-during-request-lifetime' + ? { + policy: 'deduplicate-until-invalidated', + deduplicationKey: p.deduplicationKey, + } + : p; + } + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch<{ hi: number }>(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + + const r1 = await dataSource.getFoo(1); + r1.parsedBody.hi = 99; + const r2 = await dataSource.getFoo(1); + expect(r1.parsedBody.hi).toEqual(99); + expect(r2.parsedBody.hi).toEqual(42); + expect(cloneCount).toEqual(2); + }); + + // Opting out of cloning under `deduplicate-until-invalidated` is the + // most dangerous mode: the shared parsed result stays cached and later + // callers receive the same object. Mutations are intentionally visible + // across sequential callers (aliased, not isolated). + it('does not clone under deduplicate-until-invalidated when shouldCloneParsedBodyForDeduplication is false', async () => { + let cloneCount = 0; + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + protected override requestDeduplicationPolicyFor( + url: URL, + request: WithRequired, + ): RequestDeduplicationPolicy { + const p = super.requestDeduplicationPolicyFor(url, request); + return p.policy === 'deduplicate-during-request-lifetime' + ? { + policy: 'deduplicate-until-invalidated', + deduplicationKey: p.deduplicationKey, + } + : p; + } + + protected override shouldCloneParsedBodyForDeduplication(): boolean { + return false; + } + + protected override cloneParsedBody(parsedBody: TResult) { + cloneCount += 1; + return super.cloneParsedBody(parsedBody); + } + + getFoo(id: number) { + return this.fetch<{ hi: number }>(`foo/${id}`); + } + })(); + + nock(apiUrl).get('/foo/1').reply(200, { hi: 42 }); + + const r1 = await dataSource.getFoo(1); + r1.parsedBody.hi = 99; + const r2 = await dataSource.getFoo(1); + expect(cloneCount).toEqual(0); + expect(r2.parsedBody).toBe(r1.parsedBody); + expect(r2.parsedBody.hi).toEqual(99); + }); + it('does not deduplicate non-GET requests by default', async () => { const dataSource = new (class extends RESTDataSource { override baseURL = 'https://api.example.com';