From 258cc087171e95ca10514730540113ac119d270b Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Thu, 17 Sep 2020 15:17:44 +0100 Subject: [PATCH] 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 Co-authored-by: Eric Mc Sween --- libraries/o-error/README.md | 20 ++++++ libraries/o-error/index.d.ts | 57 ++++++++-------- libraries/o-error/index.js | 26 +++++++ libraries/o-error/package.json | 9 +-- libraries/o-error/test/o-error-util.test.js | 76 +++++++++++++++++++++ 5 files changed, 157 insertions(+), 31 deletions(-) diff --git a/libraries/o-error/README.md b/libraries/o-error/README.md index 7ecea2dc76..e5944432ea 100644 --- a/libraries/o-error/README.md +++ b/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) ⇒ this * [.withCause(cause)](#OError+withCause) ⇒ this * _static_ + * [.maxTags](#OError.maxTags) : Number * [.tag(error, [message], [info])](#OError.tag) ⇒ Error * [.getFullInfo(error)](#OError.getFullInfo) ⇒ Object * [.getFullStack(error)](#OError.getFullStack) ⇒ string @@ -379,6 +388,17 @@ Wrap the given error, which caused this error. | --- | --- | --- | | cause | Error | the internal error that caused this error | + + +### OError.maxTags : Number +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 [OError](#OError) ### OError.tag(error, [message], [info]) ⇒ Error diff --git a/libraries/o-error/index.d.ts b/libraries/o-error/index.d.ts index 6d46db33ec..44332e887b 100644 --- a/libraries/o-error/index.d.ts +++ b/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 | 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 | 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; } diff --git a/libraries/o-error/index.js b/libraries/o-error/index.js index 515e39574c..8af8d1deba 100644 --- a/libraries/o-error/index.js +++ b/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 diff --git a/libraries/o-error/package.json b/libraries/o-error/package.json index ee258c21af..552ec5e667 100644 --- a/libraries/o-error/package.json +++ b/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", diff --git a/libraries/o-error/test/o-error-util.test.js b/libraries/o-error/test/o-error-util.test.js index 28f26354f0..b82c1093e6 100644 --- a/libraries/o-error/test/o-error-util.test.js +++ b/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 () {