diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index a713238e99a..e94b9bb1fb3 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -26,6 +26,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 * fix(browser-detector): use the right semantic convention for user agent resource attribute [#6729](https://github.com/open-telemetry/opentelemetry-js/pull/6729) @david-luna * fix(browser-detector): user agent resource attribute always [#6754](https://github.com/open-telemetry/opentelemetry-js/pull/6754) @david-luna * fix(opentelemetry-exporter-prometheus): handle additional edge cases in metric name conversion [#6727](https://github.com/open-telemetry/opentelemetry-js/pull/6727) @cjihrig +* fix(sdk-logs): avoid null dereference in `BatchLogRecordProcessor._flushAll` when an in-flight export completes between awaits [#6763](https://github.com/open-telemetry/opentelemetry-js/pull/6763) @Janealter ### :books: Documentation diff --git a/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts b/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts index 3b3f95e4014..e84dbd59bf4 100644 --- a/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts +++ b/experimental/packages/sdk-logs/src/export/BatchLogRecordProcessorBase.ts @@ -209,11 +209,14 @@ export abstract class BatchLogRecordProcessorBase // Clear timer to prevent concurrent exports this._clearTimer(); - // Wait for any in-progress export to complete - if (this._currentExport !== null) { + // Wait for any in-progress export to complete. Capture the reference + // into a local because `_exportOneBatch` may null out `this._currentExport` + // from its completion handler while we are awaiting below. + const inFlight = this._currentExport; + if (inFlight !== null) { // speed up execution for current export await this._exporter.forceFlush(); - await this._currentExport.exportCompleted; + await inFlight.exportCompleted; this._currentExport = null; } diff --git a/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts b/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts index 13896482355..e1f7b3fee9f 100644 --- a/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts +++ b/experimental/packages/sdk-logs/test/common/export/BatchLogRecordProcessor.test.ts @@ -537,6 +537,68 @@ describe('BatchLogRecordProcessorBase', () => { await processor.shutdown(); }); + it('should not throw when in-flight export completes between awaits in _flushAll', async () => { + // arrange + // Reproduces a race in _flushAll: when forceFlush() is called while a + // background export is in progress, _flushAll captures the in-flight + // export via `this._currentExport`. While awaiting `_exporter.forceFlush()`, + // the in-flight export may complete and `_exportOneBatch`'s completion + // handler nullifies `_currentExport`. When `_flushAll` resumes and reads + // `this._currentExport.exportCompleted`, it would dereference null. + let exportCallback: ((result: ExportResult) => void) | undefined; + let resolveExporterForceFlush: (() => void) | undefined; + const customExporter: LogRecordExporter = { + export: (_logs, callback) => { + // keep export pending so we can resolve it later + exportCallback = callback; + }, + shutdown: async () => {}, + forceFlush: () => + new Promise(resolve => { + // keep exporter.forceFlush() pending so _flushAll stays in await + resolveExporterForceFlush = resolve; + }), + }; + const processor = new BatchLogRecordProcessor(customExporter, { + maxExportBatchSize: 1, + scheduledDelayMillis: 2500, + }); + + // trigger a background export by filling a batch + processor.onEmit(createLogRecord()); + // yield so _exportOneBatch starts and exporter.export() is called + await new Promise(resolve => setTimeout(resolve, 0)); + assert.ok( + exportCallback !== undefined, + 'expected exporter.export() to have been called' + ); + + // call forceFlush — _flushAll will await exporter.forceFlush() + const flushPromise = processor.forceFlush(); + // yield so _flushAll enters its await on exporter.forceFlush() + await new Promise(resolve => setTimeout(resolve, 0)); + assert.ok( + resolveExporterForceFlush !== undefined, + 'expected exporter.forceFlush() to have been called' + ); + + // complete the in-flight export — _exportOneBatch's .then handler + // will nullify _currentExport before _flushAll resumes + exportCallback({ code: ExportResultCode.SUCCESS }); + // yield to let the .then() handler run and set _currentExport = null + await new Promise(resolve => setTimeout(resolve, 0)); + + // now resume _flushAll by resolving the exporter.forceFlush() promise. + // After the bug, _flushAll would read `this._currentExport.exportCompleted` + // and throw `Cannot read properties of null (reading 'exportCompleted')`. + resolveExporterForceFlush(); + + // assert — forceFlush must resolve, not reject with TypeError + await flushPromise; + + await processor.shutdown(); + }); + it('should not call forceFlush on exporter when queue is empty and no export in progress', async function () { // arrange const forceFlushSpy = sinon.spy(exporter, 'forceFlush');