Add a limit on the number of tags, OError.maxTags (#13)

* Add OError.maxTags

* Tidy up package scripts and add a build script

Co-authored-by: Jakob Ackermann <jakob.ackermann@overleaf.com>
Co-authored-by: Eric Mc Sween <eric.mcsween@overleaf.com>

libraries/o-error/README.md

@@ -21,6 +21,7 @@ Light-weight helpers for handling JavaScript Errors in node.js and the browser.
   * [Adding More Info](#adding-more-info)
   * [`async`/`await`](#asyncawait)
   * [Better Async Stack Traces in Node 12+](#better-async-stack-traces-in-node-12)
+  * [Caveat: Shared Error Instances](#caveat-shared-error-instances)
 - [Create Custom Error Classes](#create-custom-error-classes)
   * [Attaching Extra Info](#attaching-extra-info)
   * [Wrapping an Internal Error](#wrapping-an-internal-error)
@@ -28,6 +29,7 @@ Light-weight helpers for handling JavaScript Errors in node.js and the browser.
   * [new OError(message, [info], [cause])](#new-oerrormessage-info-cause)
   * [oError.withInfo(info) ⇒ this](#oerrorwithinfoinfo--this)
   * [oError.withCause(cause) ⇒ this](#oerrorwithcausecause--this)
+  * [OError.maxTags : Number](#oerrormaxtags--number)
   * [OError.tag(error, [message], [info]) ⇒ Error](#oerrortagerror-message-info--error)
   * [OError.getFullInfo(error) ⇒ Object](#oerrorgetfullinfoerror--object)
   * [OError.getFullStack(error) ⇒ string](#oerrorgetfullstackerror--string)
@@ -222,6 +224,12 @@ TaggedError: failed to say hi
 
 The above output is from node 10. Node 12 has improved stack traces for async code that uses native promises. However, until your whole stack, including all libraries, is using async/await and native promises, you're still likely to get unhelpful stack traces. So, the tagging approach still adds value, even in node 12. (And the `info` from tagging can add value even to a good stack trace, because it can contain clues about the input the caused the error.)
 
+### Caveat: Shared Error Instances
+
+Some libraries, such as `ioredis`, may return the same `Error` instance to multiple callbacks. In this case, the tags may be misleading, because they will be a mixture of the different 'stacks' that lead to the error. You can either accept this or choose to instead wrap the errors from these libraries with new `OError` instances using `withCause`.
+
+In the worst case, a library that always returns a single instance of an error could cause a resource leak. To prevent this, `OError` will only add up to `OError.maxTags` (default 100) tags to a single Error instance.
+
 ## Create Custom Error Classes
 
 Broadly speaking, there are two kinds of errors: those we try to recover from, and those for which we give up (i.e. a 5xx response in a web application). For the latter kind, we usually just want to log a message and stack trace useful for debugging, which `OError.tag` helps with.
@@ -343,6 +351,7 @@ caused by:
         * [.withInfo(info)](#OError+withInfo) ⇒ <code>this</code>
         * [.withCause(cause)](#OError+withCause) ⇒ <code>this</code>
     * _static_
+        * [.maxTags](#OError.maxTags) : <code>Number</code>
         * [.tag(error, [message], [info])](#OError.tag) ⇒ <code>Error</code>
         * [.getFullInfo(error)](#OError.getFullInfo) ⇒ <code>Object</code>
         * [.getFullStack(error)](#OError.getFullStack) ⇒ <code>string</code>
@@ -379,6 +388,17 @@ Wrap the given error, which caused this error.
 | --- | --- | --- |
 | cause | <code>Error</code> | the internal error that caused this error |
 
+<a name="OError.maxTags"></a>
+
+### OError.maxTags : <code>Number</code>
+Maximum number of tags to apply to any one error instance. This is to avoid
+a resource leak in the (hopefully unlikely) case that a singleton error
+instance is returned to many callbacks. If tags have been dropped, the full
+stack trace will include a placeholder tag `... dropped tags`.
+
+Defaults to 100. Must be at least 1.
+
+**Kind**: static property of [<code>OError</code>](#OError)  
 <a name="OError.tag"></a>
 
 ### OError.tag(error, [message], [info]) ⇒ <code>Error</code>

libraries/o-error/index.d.ts

@@ -4,6 +4,32 @@ export = OError;
  * browser.
  */
 declare class OError extends Error {
+    /**
+     * @param {string} message as for built-in Error
+     * @param {Object} [info] extra data to attach to the error
+     * @param {Error} [cause] the internal error that caused this error
+     */
+    constructor(message: string, info?: any, cause?: Error);
+    info: any;
+    cause: Error;
+    /** @private @type {Array<TaggedError> | undefined} */
+    private _oErrorTags;
+    /**
+     * Set the extra info object for this error.
+     *
+     * @param {Object} info extra data to attach to the error
+     * @return {this}
+     */
+    withInfo(info: any): OError;
+    /**
+     * Wrap the given error, which caused this error.
+     *
+     * @param {Error} cause the internal error that caused this error
+     * @return {this}
+     */
+    withCause(cause: Error): OError;
+}
+declare namespace OError {
     /**
      * Tag debugging information onto any error (whether an OError or not) and
      * return it.
@@ -35,7 +61,7 @@ declare class OError extends Error {
      * @param {Object} [info] extra data with wich to tag `error`
      * @return {Error} the modified `error` argument
      */
-    static tag(error: Error, message?: string, info?: any): Error;
+    export function tag(error: Error, message?: string, info?: any): Error;
     /**
      * The merged info from any `tag`s on the given error.
      *
@@ -44,7 +70,7 @@ declare class OError extends Error {
      * @param {Error | null | undefined} error any errror (may or may not be an `OError`)
      * @return {Object}
      */
-    static getFullInfo(error: Error): any;
+    export function getFullInfo(error: Error): any;
     /**
      * Return the `stack` property from `error`, including the `stack`s for any
      * tagged errors added with `OError.tag` and for any `cause`s.
@@ -52,29 +78,6 @@ declare class OError extends Error {
      * @param {Error | null | undefined} error any error (may or may not be an `OError`)
      * @return {string}
      */
-    static getFullStack(error: Error): string;
-    /**
-     * @param {string} message as for built-in Error
-     * @param {Object} [info] extra data to attach to the error
-     * @param {Error} [cause] the internal error that caused this error
-     */
-    constructor(message: string, info?: any, cause?: Error);
-    info: any;
-    cause: Error;
-    /** @private @type {Array<TaggedError> | undefined} */
-    private _oErrorTags;
-    /**
-     * Set the extra info object for this error.
-     *
-     * @param {Object} info extra data to attach to the error
-     * @return {this}
-     */
-    withInfo(info: any): OError;
-    /**
-     * Wrap the given error, which caused this error.
-     *
-     * @param {Error} cause the internal error that caused this error
-     * @return {this}
-     */
-    withCause(cause: Error): OError;
+    export function getFullStack(error: Error): string;
+    export const maxTags: Number;
 }

libraries/o-error/index.js

@@ -84,6 +84,14 @@ class OError extends Error {
       tag = new TaggedError(message || '', info)
     }
 
+    if (oError._oErrorTags.length >= OError.maxTags) {
+      // Preserve the first tag and add an indicator that we dropped some tags.
+      if (oError._oErrorTags[1] === DROPPED_TAGS_ERROR) {
+        oError._oErrorTags.splice(2, 1)
+      } else {
+        oError._oErrorTags[1] = DROPPED_TAGS_ERROR
+      }
+    }
     oError._oErrorTags.push(tag)
 
     return error
@@ -142,6 +150,18 @@ class OError extends Error {
   }
 }
 
+/**
+ * Maximum number of tags to apply to any one error instance. This is to avoid
+ * a resource leak in the (hopefully unlikely) case that a singleton error
+ * instance is returned to many callbacks. If tags have been dropped, the full
+ * stack trace will include a placeholder tag `... dropped tags`.
+ *
+ * Defaults to 100. Must be at least 1.
+ *
+ * @type {Number}
+ */
+OError.maxTags = 100
+
 /**
  * Used to record a stack trace every time we tag info onto an Error.
  *
@@ -150,6 +170,12 @@ class OError extends Error {
  */
 class TaggedError extends OError {}
 
+const DROPPED_TAGS_ERROR = /** @type{TaggedError} */ ({
+  name: 'TaggedError',
+  message: '... dropped tags',
+  stack: 'TaggedError: ... dropped tags',
+})
+
 /**
  * @private
  * @param {string} string

libraries/o-error/package.json

@@ -18,14 +18,15 @@
     "index.d.ts"
   ],
   "scripts": {
+    "build": "npm run --silent typecheck && npm run --silent test && npm run --silent declaration:build && npm run --silent update-readme",
+    "declaration:build": "rm -f index.d.ts && tsc --allowJs --declaration --emitDeclarationOnly --moduleResolution node --target ES6 index.js",
+    "declaration:check": "git diff --exit-code -- index.d.ts",
     "lint": "eslint .",
-    "update-readme": "doc/update-readme.js",
+    "prepublishOnly": "npm run --silent declaration:build && npm run --silent declaration:check",
     "test": "mocha",
     "test:coverage": "nyc --reporter=lcov --reporter=text-summary npm run test",
     "typecheck": "tsc --allowJs --checkJs --noEmit --moduleResolution node --strict --target ES6 *.js test/**/*.js",
-    "declaration:build": "rm -f index.d.ts && tsc --allowJs --declaration --emitDeclarationOnly --moduleResolution node --target ES6 index.js",
-    "declaration:check": "git diff --exit-code -- index.d.ts",
-    "prepublishOnly": "npm run --silent declaration:build && npm run --silent declaration:check"
+    "update-readme": "doc/update-readme.js"
   },
   "author": "Overleaf (https://www.overleaf.com)",
   "license": "MIT",

libraries/o-error/test/o-error-util.test.js

@@ -180,6 +180,82 @@ describe('OError.tag', function () {
       expect(stack).to.match(/TaggedError: test message\n\s+at [\w.]*tag/)
     })
   })
+
+  describe('with limit on the number of tags', function () {
+    before(function () {
+      this.originalMaxTags = OError.maxTags
+      OError.maxTags = 3
+    })
+    after(function () {
+      OError.maxTags = this.originalMaxTags
+    })
+
+    it('should not tag more than that', function () {
+      const err = new Error('test error')
+      OError.tag(err, 'test message 1')
+      OError.tag(err, 'test message 2')
+      OError.tag(err, 'test message 3')
+      OError.tag(err, 'test message 4')
+      OError.tag(err, 'test message 5')
+      expectFullStackWithoutStackFramesToEqual(err, [
+        'Error: test error',
+        'TaggedError: test message 1',
+        'TaggedError: ... dropped tags',
+        'TaggedError: test message 4',
+        'TaggedError: test message 5',
+      ])
+    })
+
+    it('should handle deep recursion', async function () {
+      async function recursiveAdd(n) {
+        try {
+          if (n === 0) throw new Error('deep error')
+          const result = await recursiveAdd(n - 1)
+          return result + 1
+        } catch (err) {
+          throw OError.tag(err, `at level ${n}`)
+        }
+      }
+      try {
+        await recursiveAdd(10)
+      } catch (err) {
+        expectFullStackWithoutStackFramesToEqual(err, [
+          'Error: deep error',
+          'TaggedError: at level 0',
+          'TaggedError: ... dropped tags',
+          'TaggedError: at level 9',
+          'TaggedError: at level 10',
+        ])
+      }
+    })
+
+    it('should handle a singleton error', function (done) {
+      const err = new Error('singleton error')
+      function endpoint(callback) {
+        helper((err) => callback(err && OError.tag(err, 'in endpoint')))
+      }
+      function helper(callback) {
+        libraryFunction((err) => callback(err && OError.tag(err, 'in helper')))
+      }
+      function libraryFunction(callback) {
+        callback(err)
+      }
+
+      endpoint(() => {
+        endpoint((err) => {
+          expect(err).to.exist
+          expectFullStackWithoutStackFramesToEqual(err, [
+            'Error: singleton error',
+            'TaggedError: in helper',
+            'TaggedError: ... dropped tags',
+            'TaggedError: in helper',
+            'TaggedError: in endpoint',
+          ])
+          done()
+        })
+      })
+    })
+  })
 })
 
 describe('OError.getFullInfo', function () {