Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,14 @@ export abstract class BatchLogRecordProcessorBase<T extends BufferConfig>
// 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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>(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');
Expand Down
Loading