From 508e921418cef286a021bea8840903ecb46abe3b Mon Sep 17 00:00:00 2001 From: Bethany Berkowitz Date: Fri, 18 Aug 2023 11:42:35 -0400 Subject: [PATCH 1/3] Move unhandled rejection monitor to a class --- packages/js/src/server.ts | 2 +- .../integrations/unhandled_rejection.ts | 44 ---------- .../unhandled_rejection_monitor.ts | 42 ++++++++++ .../unhandled_rejection_plugin.ts | 17 ++++ .../uncaught_exception_monitor.server.test.ts | 10 +-- ...unhandled_rejection_monitor.server.test.ts | 84 +++++++++++++++++++ .../unhandled_rejection_plugin.server.test.ts | 55 ++++++++++++ 7 files changed, 204 insertions(+), 50 deletions(-) delete mode 100644 packages/js/src/server/integrations/unhandled_rejection.ts create mode 100644 packages/js/src/server/integrations/unhandled_rejection_monitor.ts create mode 100644 packages/js/src/server/integrations/unhandled_rejection_plugin.ts create mode 100644 packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts create mode 100644 packages/js/test/unit/server/integrations/unhandled_rejection_plugin.server.test.ts diff --git a/packages/js/src/server.ts b/packages/js/src/server.ts index 2ef2c72f3..da48bd538 100644 --- a/packages/js/src/server.ts +++ b/packages/js/src/server.ts @@ -3,7 +3,7 @@ import domain from 'domain' import { Client, Util, Types } from '@honeybadger-io/core' import { getSourceFile } from './server/util' import uncaughtException from './server/integrations/uncaught_exception_plugin' -import unhandledRejection from './server/integrations/unhandled_rejection' +import unhandledRejection from './server/integrations/unhandled_rejection_plugin' import { errorHandler, requestHandler } from './server/middleware' import { lambdaHandler } from './server/aws_lambda' import { ServerTransport } from './server/transport' diff --git a/packages/js/src/server/integrations/unhandled_rejection.ts b/packages/js/src/server/integrations/unhandled_rejection.ts deleted file mode 100644 index f78ad87f7..000000000 --- a/packages/js/src/server/integrations/unhandled_rejection.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { Types } from '@honeybadger-io/core' -import Client from '../../server' -import { fatallyLogAndExit } from '../util' - -let isReporting = false - -/** - * If there are no other unhandledRejection listeners, - * we want to report the exception to Honeybadger and - * mimic the default behavior of NodeJs, - * which is to exit the process with code 1 - */ -function hasOtherUnhandledRejectionListeners() { - return process.listeners('unhandledRejection').length > 1 -} - -export default function (): Types.Plugin { - return { - load: (client: typeof Client) => { - if (!client.config.enableUnhandledRejection) { - return - } - - process.on('unhandledRejection', function honeybadgerUnhandledRejectionListener(reason, _promise) { - if (!client.config.enableUnhandledRejection) { - if (!hasOtherUnhandledRejectionListeners() && !isReporting) { - fatallyLogAndExit(reason as Error) - } - return - } - - isReporting = true; - client.notify(reason as Types.Noticeable, { component: 'unhandledRejection' }, { - afterNotify: () => { - isReporting = false; - if (!hasOtherUnhandledRejectionListeners()) { - fatallyLogAndExit(reason as Error) - } - } - }) - }) - } - } -} diff --git a/packages/js/src/server/integrations/unhandled_rejection_monitor.ts b/packages/js/src/server/integrations/unhandled_rejection_monitor.ts new file mode 100644 index 000000000..83df8cff5 --- /dev/null +++ b/packages/js/src/server/integrations/unhandled_rejection_monitor.ts @@ -0,0 +1,42 @@ +import Client from '../../server' +import { fatallyLogAndExit } from '../util' +import { Types } from '@honeybadger-io/core' + +export default class UnhandledRejectionMonitor { + protected __isReporting: boolean + protected __client: typeof Client + + constructor(client: typeof Client) { + this.__isReporting = false + this.__client = client + } + + /** + * If there are no other unhandledRejection listeners, + * we want to report the exception to Honeybadger and + * mimic the default behavior of NodeJs, + * which is to exit the process with code 1 + */ + hasOtherUnhandledRejectionListeners() { + return process.listeners('unhandledRejection').length > 1 + } + + handleUnhandledRejection(reason: Error | any, _promise: Promise) { + if (!this.__client.config.enableUnhandledRejection) { + if (!this.hasOtherUnhandledRejectionListeners() && !this.__isReporting) { + fatallyLogAndExit(reason as Error) + } + return + } + + this.__isReporting = true; + this.__client.notify(reason as Types.Noticeable, { component: 'unhandledRejection' }, { + afterNotify: () => { + this.__isReporting = false; + if (!this.hasOtherUnhandledRejectionListeners()) { + fatallyLogAndExit(reason as Error) + } + } + }) + } +} diff --git a/packages/js/src/server/integrations/unhandled_rejection_plugin.ts b/packages/js/src/server/integrations/unhandled_rejection_plugin.ts new file mode 100644 index 000000000..3f2f3b65b --- /dev/null +++ b/packages/js/src/server/integrations/unhandled_rejection_plugin.ts @@ -0,0 +1,17 @@ +import { Types } from '@honeybadger-io/core' +import Client from '../../server' +import UnhandledRejectionMonitor from './unhandled_rejection_monitor' + +export default function (): Types.Plugin { + return { + load: (client: typeof Client) => { + if (!client.config.enableUnhandledRejection) { + return + } + const unhandledRejectionMonitor = new UnhandledRejectionMonitor(client) + process.on('unhandledRejection', function honeybadgerUnhandledRejectionListener(reason, _promise) { + unhandledRejectionMonitor.handleUnhandledRejection(reason, _promise) + }) + } + } +} diff --git a/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts b/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts index 2eae7012a..0684a40d9 100644 --- a/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts +++ b/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts @@ -55,7 +55,7 @@ describe('UncaughtExceptionMonitor', () => { const error = new Error('dang, broken again') it('calls notify, afterUncaught, and fatallyLogAndExit', (done) => { - uncaughtExceptionMonitor.handleUncaughtException(error, client) + uncaughtExceptionMonitor.handleUncaughtException(error) expect(notifySpy).toHaveBeenCalledTimes(1) expect(notifySpy).toHaveBeenCalledWith( error, @@ -70,7 +70,7 @@ describe('UncaughtExceptionMonitor', () => { it('returns if it is already reporting', () => { uncaughtExceptionMonitor.__isReporting = true - uncaughtExceptionMonitor.handleUncaughtException(error, client) + uncaughtExceptionMonitor.handleUncaughtException(error) expect(notifySpy).not.toHaveBeenCalled() expect(fatallyLogAndExitSpy).not.toHaveBeenCalled() }) @@ -78,21 +78,21 @@ describe('UncaughtExceptionMonitor', () => { it('returns if it was already called and there are other listeners', () => { process.on('uncaughtException', () => true) process.on('uncaughtException', () => true) - uncaughtExceptionMonitor.handleUncaughtException(error, client) + uncaughtExceptionMonitor.handleUncaughtException(error) expect(notifySpy).toHaveBeenCalledTimes(1) client.afterNotify(() => { expect(fatallyLogAndExitSpy).not.toHaveBeenCalled() expect(uncaughtExceptionMonitor.__handlerAlreadyCalled).toBe(true) // Doesn't notify a second time - uncaughtExceptionMonitor.handleUncaughtException(error, client) + uncaughtExceptionMonitor.handleUncaughtException(error) expect(notifySpy).toHaveBeenCalledTimes(1) }) }) it('exits if it was already called and there are no other listeners', () => { uncaughtExceptionMonitor.__handlerAlreadyCalled = true - uncaughtExceptionMonitor.handleUncaughtException(error, client) + uncaughtExceptionMonitor.handleUncaughtException(error) expect(notifySpy).not.toHaveBeenCalled() expect(fatallyLogAndExitSpy).toHaveBeenCalledWith(error) }) diff --git a/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts b/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts new file mode 100644 index 000000000..7487ac6f6 --- /dev/null +++ b/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts @@ -0,0 +1,84 @@ +import { TestTransport, TestClient, nullLogger } from '../../helpers' +import * as util from '../../../../src/server/util' +import Singleton from '../../../../src/server' +import UnhandledRejectionMonitor from '../../../../src/server/integrations/unhandled_rejection_monitor' + + +describe('UnhandledRejectionMonitor', () => { + // Using any rather than the real type so we can test and spy on private methods + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let unhandledRejectionMonitor: any + let client: typeof Singleton + let fatallyLogAndExitSpy: jest.SpyInstance + let notifySpy: jest.SpyInstance + + beforeEach(() => { + // We just need a really basic client, so ignoring type issues here + client = new TestClient( + { apiKey: 'testKey', afterUncaught: jest.fn(), logger: nullLogger() }, + new TestTransport() + ) as unknown as typeof Singleton + unhandledRejectionMonitor = new UnhandledRejectionMonitor(client) + // Have to mock fatallyLogAndExit or we will crash the test + fatallyLogAndExitSpy = jest + .spyOn(util, 'fatallyLogAndExit') + .mockImplementation(() => true as never) + notifySpy = jest.spyOn(client, 'notify') + }) + + afterEach(() => { + jest.clearAllMocks() + process.removeAllListeners('unhandledRejection') + unhandledRejectionMonitor.__isReporting = false + }) + + describe('constructor', () => { + it('set up variables and client', () => { + expect(unhandledRejectionMonitor.__isReporting).toBe(false) + expect(unhandledRejectionMonitor.__client.config.apiKey).toBe('testKey') + }) + }) + + describe('handleUnhandledRejection', () => { + const promise = new Promise(() => true) + const reason = 'Promise rejection reason' + + it('calls notify and fatallyLogAndExit', (done) => { + unhandledRejectionMonitor.handleUnhandledRejection(reason, promise) + expect(notifySpy).toHaveBeenCalledTimes(1) + expect(notifySpy).toHaveBeenCalledWith( + reason, + { component: 'unhandledRejection' }, + { afterNotify: expect.any(Function) } + ) + client.afterNotify(() => { + expect(fatallyLogAndExitSpy).toHaveBeenCalledWith(reason) + done() + }) + }) + + // Seems like it was meant to work this way but was never implemented? + // it('returns if it is already reporting', () => { + // unhandledRejectionMonitor.__isReporting = true + // unhandledRejectionMonitor.handleUnhandledRejection(reason, promise) + // expect(notifySpy).not.toHaveBeenCalled() + // expect(fatallyLogAndExitSpy).not.toHaveBeenCalled() + // }) + + it('exits if enableUnhandledRejection is false and there are no other listeners', () => { + client.configure({ enableUnhandledRejection: false }) + unhandledRejectionMonitor.handleUnhandledRejection(reason, promise) + expect(notifySpy).not.toHaveBeenCalled() + expect(fatallyLogAndExitSpy).toHaveBeenCalledWith(reason) + }) + + it('returns if enableUnhandledRejection is false and there are other listeners', () => { + process.on('unhandledRejection', () => true) + process.on('unhandledRejection', () => true) + client.configure({ enableUnhandledRejection: false }) + unhandledRejectionMonitor.handleUnhandledRejection(reason, promise) + expect(notifySpy).not.toHaveBeenCalled() + expect(fatallyLogAndExitSpy).not.toHaveBeenCalled() + }) + }) +}) diff --git a/packages/js/test/unit/server/integrations/unhandled_rejection_plugin.server.test.ts b/packages/js/test/unit/server/integrations/unhandled_rejection_plugin.server.test.ts new file mode 100644 index 000000000..edbfe28c8 --- /dev/null +++ b/packages/js/test/unit/server/integrations/unhandled_rejection_plugin.server.test.ts @@ -0,0 +1,55 @@ +import plugin from '../../../../src/server/integrations/unhandled_rejection_plugin' +import { TestTransport, TestClient, nullLogger } from '../../helpers' +import * as util from '../../../../src/server/util' +import Singleton from '../../../../src/server' + +describe('Uncaught Exception Plugin', () => { + let client: typeof Singleton + let notifySpy: jest.SpyInstance + + beforeEach(() => { + // We just need a really basic client, so ignoring type issues here + client = new TestClient( + { apiKey: 'testKey', afterUncaught: jest.fn(), logger: nullLogger() }, + new TestTransport() + ) as unknown as typeof Singleton + // Have to mock fatallyLogAndExit or we will crash the test + jest + .spyOn(util, 'fatallyLogAndExit') + .mockImplementation(() => true as never) + notifySpy = jest.spyOn(client, 'notify') + }) + + afterEach(() => { + jest.clearAllMocks() + process.removeAllListeners('unhandledRejection') + }) + + it('is a function which returns an object with a load function', () => { + expect(plugin()).toStrictEqual({ + load: expect.any(Function) + }) + }) + + describe('load', () => { + const load = plugin().load + + it('attaches a listener for unhandledRejection', () => { + load(client) + const listeners = process.listeners('unhandledRejection') + expect(listeners.length).toBe(1) + expect(listeners[0].name).toBe('honeybadgerUnhandledRejectionListener') + + const promise = new Promise(() => true) + process.emit('unhandledRejection', 'Stuff went wrong', promise) + expect(notifySpy).toHaveBeenCalledTimes(1) + }) + + it('returns if enableUncaught is not true', () => { + client.configure({ enableUnhandledRejection: false }) + load(client) + const listeners = process.listeners('unhandledRejection') + expect(listeners.length).toBe(0) + }) + }) +}) From ce2defdafab3b26f5cc6fe475079f118e0ed94c9 Mon Sep 17 00:00:00 2001 From: Bethany Berkowitz Date: Fri, 18 Aug 2023 15:34:41 -0400 Subject: [PATCH 2/3] Test hasOtherUncaughtExceptionListeners --- .../uncaught_exception_monitor.ts | 2 +- .../uncaught_exception_monitor.server.test.ts | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/js/src/server/integrations/uncaught_exception_monitor.ts b/packages/js/src/server/integrations/uncaught_exception_monitor.ts index 18d8b3703..36e8345bf 100644 --- a/packages/js/src/server/integrations/uncaught_exception_monitor.ts +++ b/packages/js/src/server/integrations/uncaught_exception_monitor.ts @@ -31,7 +31,7 @@ export default class UncaughtExceptionMonitor { // Node sets up these listeners when we use domains // Since they're not set up by a user, they shouldn't affect whether we exit or not const domainListeners = allListeners.filter(listener => { - listener.name === 'domainUncaughtExceptionClear' + return listener.name === 'domainUncaughtExceptionClear' }) return allListeners.length - domainListeners.length > 1 } diff --git a/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts b/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts index 0684a40d9..94eae6d08 100644 --- a/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts +++ b/packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts @@ -97,4 +97,29 @@ describe('UncaughtExceptionMonitor', () => { expect(fatallyLogAndExitSpy).toHaveBeenCalledWith(error) }) }) + + describe('hasOtherUncaughtExceptionListeners', () => { + it('returns true if there are user-added listeners', () => { + process.on('uncaughtException', function honeybadgerUncaughtExceptionListener() { + return + }) + process.on('uncaughtException', function domainUncaughtExceptionClear() { + return + }) + process.on('uncaughtException', function userAddedListener() { + return + }) + expect(uncaughtExceptionMonitor.hasOtherUncaughtExceptionListeners()).toBe(true) + }) + + it('returns false if there are only our expected listeners', () => { + process.on('uncaughtException', function honeybadgerUncaughtExceptionListener() { + return + }) + process.on('uncaughtException', function domainUncaughtExceptionClear() { + return + }) + expect(uncaughtExceptionMonitor.hasOtherUncaughtExceptionListeners()).toBe(false) + }) + }) }) From 21d8d849b18a3a541b1f0c65b562dda82b2fc9c4 Mon Sep 17 00:00:00 2001 From: Bethany Berkowitz Date: Fri, 18 Aug 2023 15:43:20 -0400 Subject: [PATCH 3/3] Clean up and lint --- .../server/integrations/unhandled_rejection_monitor.ts | 2 +- .../unhandled_rejection_monitor.server.test.ts | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/js/src/server/integrations/unhandled_rejection_monitor.ts b/packages/js/src/server/integrations/unhandled_rejection_monitor.ts index 83df8cff5..bc3f2f845 100644 --- a/packages/js/src/server/integrations/unhandled_rejection_monitor.ts +++ b/packages/js/src/server/integrations/unhandled_rejection_monitor.ts @@ -21,7 +21,7 @@ export default class UnhandledRejectionMonitor { return process.listeners('unhandledRejection').length > 1 } - handleUnhandledRejection(reason: Error | any, _promise: Promise) { + handleUnhandledRejection(reason: unknown, _promise: Promise) { if (!this.__client.config.enableUnhandledRejection) { if (!this.hasOtherUnhandledRejectionListeners() && !this.__isReporting) { fatallyLogAndExit(reason as Error) diff --git a/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts b/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts index 7487ac6f6..6b8ca384c 100644 --- a/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts +++ b/packages/js/test/unit/server/integrations/unhandled_rejection_monitor.server.test.ts @@ -56,14 +56,6 @@ describe('UnhandledRejectionMonitor', () => { done() }) }) - - // Seems like it was meant to work this way but was never implemented? - // it('returns if it is already reporting', () => { - // unhandledRejectionMonitor.__isReporting = true - // unhandledRejectionMonitor.handleUnhandledRejection(reason, promise) - // expect(notifySpy).not.toHaveBeenCalled() - // expect(fatallyLogAndExitSpy).not.toHaveBeenCalled() - // }) it('exits if enableUnhandledRejection is false and there are no other listeners', () => { client.configure({ enableUnhandledRejection: false })