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
@@ -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<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;
-
   // 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);

packages/node/test/context-lines.benchmark.ts

@@ -0,0 +1,84 @@
+import type { StackFrame } from '@sentry/types';
+import * as fs from 'fs';
+
+import { parseStackFrames } from '../src/eventbuilder';
+import { ContextLines, resetFileContentCache } from '../src/integrations/contextlines';
+import { defaultStackParser } from '../src/sdk';
+import { getError } from './helper/error';
+
+import * as Benchmark from 'benchmark';
+
+const lines = new ContextLines({});
+
+const source = {
+  exception: {
+    values: [
+      {
+        stacktrace: {
+          frames: [
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/requestdata.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/sdk.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/stacktrace.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/utils.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/http.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/test/transports/https.test.ts',
+              lineno: 1,
+              function: 'fxn1',
+            },
+            {
+              colno: 1,
+              filename: '/Users/jonasbadalic/code/sentry-javascript/packages/node/tsconfig.json',
+              lineno: 1,
+              function: 'fxn1',
+            },
+          ],
+        },
+      },
+    ],
+  },
+};
+
+const suite = new Benchmark.Suite({ setup: resetFileContentCache, teardown: resetFileContentCache });
+
+suite
+  .add('parallel io', async function () {
+    await lines.addSourceContext(source);
+    await lines.addSourceContext(source);
+  })
+  .on('cycle', function (event: any) {
+    console.log(String(event.target));
+  })
+  .run({ async: true });

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