ref(contextlines): store split file in cache

Author: JonasBa

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<string, string | null>(100);
+const FILE_CONTENT_CACHE = new LRUMap<string, string[] | null>(100);
 const DEFAULT_LINES_OF_CONTEXT = 7;
 
 // TODO: Replace with promisify when minimum supported node >= v8
@@ -64,7 +64,7 @@ export class ContextLines implements Integration {
   public async addSourceContext(event: Event): Promise<Event> {
     if (this._contextLines > 0 && event.exception?.values) {
       for (const exception of event.exception.values) {
-        if (exception.stacktrace?.frames) {
+        if (exception.stacktrace && exception.stacktrace.frames) {
           await this.addSourceContextToFrames(exception.stacktrace.frames);
         }
       }
@@ -80,12 +80,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 = await _readSourceFile(frame.filename);
+        const sourceFileLines = await _readSourceFile(frame.filename);
 
-        if (sourceFile) {
+        if (sourceFileLines) {
           try {
-            const lines = sourceFile.split('\n');
-            addContextToFrame(lines, frame, contextLines);
+            addContextToFrame(sourceFileLines, frame, contextLines);
           } catch (e) {
             // anomaly, being defensive in case
             // unlikely to ever happen in practice but can definitely happen in theory
@@ -98,21 +97,30 @@ 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<string | null> {
+async function _readSourceFile(filename: string): Promise<string[] | null> {
   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;
+  // File is neither in cache nor marked as failed, 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);

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);
+  });
 });