From cd31e33ede4262b475fd6ccca4fa06e98f87d434 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 7 Oct 2022 15:06:53 -0700 Subject: [PATCH] usage reporting: fix memory leak (#7000) A data structure in the usage reporting plugin would get a new entry each time the server's schema changed. Old entries would never be deleted, though their values would be shrunk to a minimum size. It seems that this was not good enough and is producing notable memory leaks for some folks. So let's fix it! The reason for the old behavior was to be really careful to never write to a report that may have already been taken out of the map and sent. Previously this was accomplished by having the main entry in the map never change. Now this is accomplished by making sure that we never write to the report any later than immediately (and synchronously) after we pull it out of the map. Fixes #6983. Backport from #6998. --- CHANGELOG.md | 3 + .../src/plugin/usageReporting/plugin.ts | 100 ++++++++++-------- 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84280eeb4b3..6149992ae92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ The version headers in this history reflect the versions of Apollo Server itself ## vNEXT +## v3.10.3 +- `apollo-server-core`: Fix memory leak in usage reporting plugin. [Issue #6983](https://github.com/apollographql/apollo-server/issues/6983) [PR #6999](https://github.com/apollographql/apollo-server/[Issue #6983](https://github.com/apollographql/apollo-server/issues/6983)pull/6999) + ## v3.10.2 - `apollo-server-fastify`: Use `return reply.send` in handlers to match the pattern encouraged by Fastify 4 (although [`apollo-server-fastify@3` only works with Fastify 3](https://github.com/apollographql/apollo-server/issues/6576#issuecomment-1159249244)). [PR #6798](https://github.com/apollographql/apollo-server/pull/6798) diff --git a/packages/apollo-server-core/src/plugin/usageReporting/plugin.ts b/packages/apollo-server-core/src/plugin/usageReporting/plugin.ts index b00a9199d5c..8bce7bd3d54 100644 --- a/packages/apollo-server-core/src/plugin/usageReporting/plugin.ts +++ b/packages/apollo-server-core/src/plugin/usageReporting/plugin.ts @@ -48,22 +48,6 @@ const reportHeaderDefaults = { uname: `${os.platform()}, ${os.type()}, ${os.release()}, ${os.arch()})`, }; -class ReportData { - report!: OurReport; - readonly header: ReportHeader; - constructor(executableSchemaId: string, graphRef: string) { - this.header = new ReportHeader({ - ...reportHeaderDefaults, - executableSchemaId, - graphRef, - }); - this.reset(); - } - reset() { - this.report = new OurReport(this.header); - } -} - export function ApolloServerPluginUsageReporting( options: ApolloServerPluginUsageReportingOptions = Object.create( null, @@ -145,9 +129,45 @@ export function ApolloServerPluginUsageReporting( cache: LRUCache; } | null = null; - const reportDataByExecutableSchemaId: { - [executableSchemaId: string]: ReportData | undefined; - } = Object.create(null); + // This map maps from executable schema ID (schema hash, basically) to the + // report we'll send about it. That's because when we're using a gateway, + // the schema can change over time, but each report needs to be about a + // single schema. We avoid having this function be a memory leak by + // removing values from it when we're in the process of sending reports. + // That means we have to be very careful never to pull a Report out of it + // and hang on to it for a while before writing to it, because the report + // might have gotten sent and discarded in the meantime. So you should + // only access the values of this Map via + // getReportWhichMustBeUsedImmediately and getAndDeleteReport, and never + // hang on to the value returned by getReportWhichMustBeUsedImmediately. + const reportByExecutableSchemaId = new Map(); + const getReportWhichMustBeUsedImmediately = ( + executableSchemaId: string, + ): OurReport => { + const existing = reportByExecutableSchemaId.get(executableSchemaId); + if (existing) { + return existing; + } + const report = new OurReport( + new ReportHeader({ + ...reportHeaderDefaults, + executableSchemaId, + graphRef, + }), + ); + reportByExecutableSchemaId.set(executableSchemaId, report); + return report; + }; + const getAndDeleteReport = ( + executableSchemaId: string, + ): OurReport | null => { + const report = reportByExecutableSchemaId.get(executableSchemaId); + if (report) { + reportByExecutableSchemaId.delete(executableSchemaId); + return report; + } + return null; + }; const overriddenExecutableSchemaId = options.overrideReportedSchema ? computeCoreSchemaHash(options.overrideReportedSchema) @@ -193,21 +213,10 @@ export function ApolloServerPluginUsageReporting( return id; } - const getReportData = (executableSchemaId: string): ReportData => { - const existing = reportDataByExecutableSchemaId[executableSchemaId]; - if (existing) { - return existing; - } - const reportData = new ReportData(executableSchemaId, graphRef); - reportDataByExecutableSchemaId[executableSchemaId] = reportData; - return reportData; - }; - async function sendAllReportsAndReportErrors(): Promise { await Promise.all( - Object.keys(reportDataByExecutableSchemaId).map( - (executableSchemaId) => - sendReportAndReportErrors(executableSchemaId), + [...reportByExecutableSchemaId.keys()].map((executableSchemaId) => + sendReportAndReportErrors(executableSchemaId), ), ); } @@ -229,13 +238,11 @@ export function ApolloServerPluginUsageReporting( // Needs to be an arrow function to be confident that key is defined. const sendReport = async (executableSchemaId: string): Promise => { - const reportData = getReportData(executableSchemaId); - const { report } = reportData; - reportData.reset(); - + const report = getAndDeleteReport(executableSchemaId); if ( - Object.keys(report.tracesPerQuery).length === 0 && - report.operationCount === 0 + !report || + (Object.keys(report.tracesPerQuery).length === 0 && + report.operationCount === 0) ) { return; } @@ -580,10 +587,11 @@ export function ApolloServerPluginUsageReporting( const executableSchemaId = overriddenExecutableSchemaId ?? executableSchemaIdForSchema(schema); - const reportData = getReportData(executableSchemaId); if (includeOperationInUsageReporting === false) { - if (resolvedOperation) reportData.report.operationCount++; + if (resolvedOperation) + getReportWhichMustBeUsedImmediately(executableSchemaId) + .operationCount++; return; } @@ -638,8 +646,6 @@ export function ApolloServerPluginUsageReporting( overriddenExecutableSchemaId ?? executableSchemaIdForSchema(schema); - const reportData = getReportData(executableSchemaId); - const { report } = reportData; const { trace } = treeBuilder; let statsReportKey: string | undefined = undefined; @@ -677,9 +683,12 @@ export function ApolloServerPluginUsageReporting( throw new Error(`Error encoding trace: ${protobufError}`); } - if (resolvedOperation) report.operationCount++; + if (resolvedOperation) { + getReportWhichMustBeUsedImmediately(executableSchemaId) + .operationCount++; + } - report.addTrace({ + getReportWhichMustBeUsedImmediately(executableSchemaId).addTrace({ statsReportKey, trace, // We include the operation as a trace (rather than aggregated @@ -705,7 +714,8 @@ export function ApolloServerPluginUsageReporting( // If the buffer gets big (according to our estimate), send. if ( sendReportsImmediately || - report.sizeEstimator.bytes >= + getReportWhichMustBeUsedImmediately(executableSchemaId) + .sizeEstimator.bytes >= (options.maxUncompressedReportSize || 4 * 1024 * 1024) ) { await sendReportAndReportErrors(executableSchemaId);