diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts index f3cebc1d4037..bad96ae43fb2 100644 --- a/packages/node/src/integrations/contextlines.ts +++ b/packages/node/src/integrations/contextlines.ts @@ -3,7 +3,7 @@ import { addContextToFrame } from '@sentry/utils'; import { readFile } from 'fs'; import { LRUMap } from 'lru_map'; -const FILE_CONTENT_CACHE = new LRUMap(100); +const FILE_CONTENT_CACHE = new LRUMap(100); const DEFAULT_LINES_OF_CONTEXT = 7; // TODO: Replace with promisify when minimum supported node >= v8 @@ -65,7 +65,7 @@ export class ContextLines implements Integration { // keep a lookup map of which files we've already enqueued to read, // so we don't enqueue the same file multiple times which would cause multiple i/o reads const enqueuedReadSourceFileTasks: Record = {}; - const readSourceFileTasks: Promise[] = []; + const readSourceFileTasks: Promise[] = []; if (this._contextLines > 0 && event.exception?.values) { for (const exception of event.exception.values) { @@ -102,8 +102,8 @@ export class ContextLines implements Integration { // and attempt to add source context to frames. if (this._contextLines > 0 && event.exception?.values) { for (const exception of event.exception.values) { - if (exception.stacktrace?.frames) { - this.addSourceContextToFrames(exception.stacktrace.frames); + if (exception.stacktrace && exception.stacktrace.frames) { + await this.addSourceContextToFrames(exception.stacktrace.frames); } } } @@ -116,12 +116,11 @@ export class ContextLines implements Integration { for (const frame of frames) { // Only add context if we have a filename and it hasn't already been added if (frame.filename && frame.context_line === undefined) { - const sourceFile = FILE_CONTENT_CACHE.get(frame.filename); + const sourceFileLines = FILE_CONTENT_CACHE.get(frame.filename); - if (sourceFile) { + if (sourceFileLines) { try { - const lines = sourceFile.split('\n'); - addContextToFrame(lines, frame, this._contextLines); + addContextToFrame(sourceFileLines, frame, this._contextLines); } catch (e) { // anomaly, being defensive in case // unlikely to ever happen in practice but can definitely happen in theory @@ -134,25 +133,34 @@ export class ContextLines implements Integration { /** * Reads file contents and caches them in a global LRU cache. + * If reading fails, mark the file as null in the cache so we don't try again. * * @param filename filepath to read content from. */ -async function _readSourceFile(filename: string): Promise { +async function _readSourceFile(filename: string): Promise { const cachedFile = FILE_CONTENT_CACHE.get(filename); - // We have a cache hit + + // We have already attempted to read this file and failed, do not try again + if (cachedFile === null) { + return null; + } + + // We have a cache hit, return it if (cachedFile !== undefined) { return cachedFile; } - let content: string | null = null; - // Guard from throwing if readFile fails, this enables us to use Promise.all and // not have it short circuiting if one of the promises rejects + since context lines are added // on a best effort basis, we want to throw here anyways. + + // If we made it to here, it means that our file is not cache nor marked as failed, so attempt to read it + let content: string[] | null = null; try { - content = await readTextFileAsync(filename); + const rawFileContents = await readTextFileAsync(filename); + content = rawFileContents.split('\n'); } catch (_) { - // + // if we fail, we will mark the file as null in the cache and short circuit next time we try to read it } FILE_CONTENT_CACHE.set(filename, content); diff --git a/packages/node/test/context-lines.test.ts b/packages/node/test/context-lines.test.ts index de6f6f382cd8..b469796214b1 100644 --- a/packages/node/test/context-lines.test.ts +++ b/packages/node/test/context-lines.test.ts @@ -107,4 +107,31 @@ describe('ContextLines', () => { expect(readFileSpy).toHaveBeenCalledTimes(0); }); }); + test.only('does not attempt to readfile multiple times if it fails', async () => { + expect.assertions(1); + contextLines = new ContextLines({}); + + readFileSpy.mockImplementation(() => { + throw new Error("ENOENT: no such file or directory, open '/does/not/exist.js'"); + }); + + await addContext([ + { + colno: 1, + filename: '/does/not/exist.js', + lineno: 1, + function: 'fxn1', + }, + ]); + await addContext([ + { + colno: 1, + filename: '/does/not/exist.js', + lineno: 1, + function: 'fxn1', + }, + ]); + + expect(readFileSpy).toHaveBeenCalledTimes(1); + }); });