chore(js): allow uncaughtException plugin to be loaded more than once

packages/js/src/server/integrations/uncaught_exception_monitor.ts

@@ -6,14 +6,68 @@ export default class UncaughtExceptionMonitor {
   protected __isReporting: boolean
   protected __handlerAlreadyCalled: boolean
   protected __client: typeof Client
+  protected __listener: (error: Error) => void
 
-  constructor(client: typeof Client) {
+  constructor() {
     this.__isReporting = false
-    this.__handlerAlreadyCalled = false
-    this.__client = client
+    this.__handlerAlreadyCalled = false 
+    this.__listener = this.makeListener()
     this.removeAwsLambdaListener()
   }
 
+  setClient(client: typeof Client) {
+    this.__client = client
+  }
+
+  makeListener() {
+    const honeybadgerUncaughtExceptionListener = (uncaughtError: Error) => {
+      if (this.__isReporting || !this.__client) { return }
+
+      if (!this.__client.config.enableUncaught) {
+        this.__client.config.afterUncaught(uncaughtError)
+        if (!this.hasOtherUncaughtExceptionListeners()) {
+          fatallyLogAndExit(uncaughtError)
+        }
+        return
+      }
+
+      // report only the first error - prevent reporting recursive errors
+      if (this.__handlerAlreadyCalled) {
+        if (!this.hasOtherUncaughtExceptionListeners()) {
+          fatallyLogAndExit(uncaughtError)
+        }
+        return
+      }
+
+      this.__isReporting = true
+      this.__client.notify(uncaughtError, {
+        afterNotify: (_err, _notice) => {
+          this.__isReporting = false
+          this.__handlerAlreadyCalled = true
+          this.__client.config.afterUncaught(uncaughtError)
+          if (!this.hasOtherUncaughtExceptionListeners()) {
+            fatallyLogAndExit(uncaughtError)
+          }
+        }
+      })
+    }
+    return honeybadgerUncaughtExceptionListener
+  }
+
+  maybeAddListener() {
+    const listeners = process.listeners('uncaughtException')
+    if (!listeners.includes(this.__listener)) {
+      process.on('uncaughtException', this.__listener)
+    }
+  }
+
+  maybeRemoveListener() {
+    const listeners = process.listeners('uncaughtException')
+    if (listeners.includes(this.__listener)) {
+      process.removeListener('uncaughtException', this.__listener)
+    }
+  }
+
   removeAwsLambdaListener() {
     const isLambda = !!process.env.LAMBDA_TASK_ROOT
     if (!isLambda) { return }
@@ -25,46 +79,16 @@ export default class UncaughtExceptionMonitor {
    * we want to report the exception to Honeybadger and
    * mimic the default behavior of NodeJs,
    * which is to exit the process with code 1
+   * 
+   * Node sets up domainUncaughtExceptionClear when we use domains.
+   * Since they're not set up by a user, they shouldn't affect whether we exit or not
    */
-  hasOtherUncaughtExceptionListeners() {
+  hasOtherUncaughtExceptionListeners(): boolean {
     const allListeners = process.listeners('uncaughtException')
-    // 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 => {
-      return listener.name === 'domainUncaughtExceptionClear' 
-    })
-    return allListeners.length - domainListeners.length > 1
-  }
-
-  handleUncaughtException(uncaughtError: Error) {
-    if (this.__isReporting) { return }
-
-    if (!this.__client.config.enableUncaught) {
-      this.__client.config.afterUncaught(uncaughtError)
-      if (!this.hasOtherUncaughtExceptionListeners()) {
-        fatallyLogAndExit(uncaughtError)
-      }
-      return
-    }
-
-    // report only the first error - prevent reporting recursive errors
-    if (this.__handlerAlreadyCalled) {
-      if (!this.hasOtherUncaughtExceptionListeners()) {
-        fatallyLogAndExit(uncaughtError)
-      }
-      return
-    }
-
-    this.__isReporting = true
-    this.__client.notify(uncaughtError, {
-      afterNotify: (_err, _notice) => {
-        this.__isReporting = false
-        this.__handlerAlreadyCalled = true
-        this.__client.config.afterUncaught(uncaughtError)
-        if (!this.hasOtherUncaughtExceptionListeners()) {
-          fatallyLogAndExit(uncaughtError)
-        }
-      }
-    })
+    const otherListeners = allListeners.filter(listener => (
+      listener.name !== 'domainUncaughtExceptionClear' 
+      && listener !== this.__listener
+    ))
+    return otherListeners.length > 0
   }
 }

packages/js/src/server/integrations/uncaught_exception_plugin.ts

@@ -2,16 +2,17 @@ import { Types } from '@honeybadger-io/core'
 import Client from '../../server'
 import UncaughtExceptionMonitor from './uncaught_exception_monitor'
 
+const uncaughtExceptionMonitor = new UncaughtExceptionMonitor()
+
 export default function (): Types.Plugin {
   return {
     load: (client: typeof Client) => {
-      if (!client.config.enableUncaught) { 
-        return
+      uncaughtExceptionMonitor.setClient(client)
+      if (client.config.enableUncaught) { 
+        uncaughtExceptionMonitor.maybeAddListener()
+      } else {
+        uncaughtExceptionMonitor.maybeRemoveListener()
       }
-      const uncaughtExceptionMonitor = new UncaughtExceptionMonitor(client)
-      process.on('uncaughtException', function honeybadgerUncaughtExceptionListener(uncaughtError) {
-        uncaughtExceptionMonitor.handleUncaughtException(uncaughtError)
-      })
     }
   }
 }

packages/js/test/unit/server/integrations/uncaught_exception_monitor.server.test.ts

@@ -4,6 +4,10 @@ import Singleton from '../../../../src/server'
 import UncaughtExceptionMonitor from '../../../../src/server/integrations/uncaught_exception_monitor'
 import * as aws from '../../../../src/server/aws_lambda'
 
+function getListenerCount() {
+  return process.listeners('uncaughtException').length
+}
+
 describe('UncaughtExceptionMonitor', () => {
   // 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
@@ -18,7 +22,8 @@ describe('UncaughtExceptionMonitor', () => {
       { apiKey: 'testKey', afterUncaught: jest.fn(), logger: nullLogger() }, 
       new TestTransport()
     ) as unknown as typeof Singleton
-    uncaughtExceptionMonitor = new UncaughtExceptionMonitor(client)
+    uncaughtExceptionMonitor = new UncaughtExceptionMonitor()
+    uncaughtExceptionMonitor.setClient(client)
     // Have to mock fatallyLogAndExit or we will crash the test
     fatallyLogAndExitSpy = jest
       .spyOn(util, 'fatallyLogAndExit')
@@ -42,20 +47,53 @@ describe('UncaughtExceptionMonitor', () => {
 
       // 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
-      const newMonitor = new UncaughtExceptionMonitor(client) as any
+      const newMonitor = new UncaughtExceptionMonitor() as any
       expect(removeLambdaSpy).toHaveBeenCalledTimes(1)
       expect(newMonitor.__isReporting).toBe(false)
       expect(newMonitor.__handlerAlreadyCalled).toBe(false)
+      expect(newMonitor.__listener).toStrictEqual(expect.any(Function))
+      expect(newMonitor.__listener.name).toBe('honeybadgerUncaughtExceptionListener')
 
       process.env = restoreEnv
     })
   })
 
-  describe('handleUncaughtException', () => {   
+  describe('maybeAddListener', () => {
+    it('adds our listener a maximum of one time', () => {
+      expect(getListenerCount()).toBe(0)
+      // Adds our listener
+      uncaughtExceptionMonitor.maybeAddListener()
+      expect(getListenerCount()).toBe(1)
+      // Doesn't add a duplicate
+      uncaughtExceptionMonitor.maybeAddListener()
+      expect(getListenerCount()).toBe(1)
+    })
+  })
+
+  describe('maybeRemoveListener', () => {
+    it('removes our listener if it is present', () => {
+      uncaughtExceptionMonitor.maybeAddListener()
+      process.on('uncaughtException', (err) => { console.log(err) })
+      expect(getListenerCount()).toBe(2)
+
+      uncaughtExceptionMonitor.maybeRemoveListener()
+      expect(getListenerCount()).toBe(1)
+    })
+
+    it('does nothing if our listener is not present', () => {
+      process.on('uncaughtException', (err) => { console.log(err) })
+      expect(getListenerCount()).toBe(1)
+
+      uncaughtExceptionMonitor.maybeRemoveListener()
+      expect(getListenerCount()).toBe(1)
+    })
+  })
+
+  describe('__listener', () => {   
     const error = new Error('dang, broken again')  
 
     it('calls notify, afterUncaught, and fatallyLogAndExit', (done) => {
-      uncaughtExceptionMonitor.handleUncaughtException(error)
+      uncaughtExceptionMonitor.__listener(error)
       expect(notifySpy).toHaveBeenCalledTimes(1)
       expect(notifySpy).toHaveBeenCalledWith(
         error, 
@@ -70,39 +108,37 @@ describe('UncaughtExceptionMonitor', () => {
 
     it('returns if it is already reporting', () => {
       uncaughtExceptionMonitor.__isReporting = true
-      uncaughtExceptionMonitor.handleUncaughtException(error)
+      uncaughtExceptionMonitor.__listener(error)
       expect(notifySpy).not.toHaveBeenCalled()
       expect(fatallyLogAndExitSpy).not.toHaveBeenCalled()
     })
 
     it('returns if it was already called and there are other listeners', () => {
       process.on('uncaughtException', () => true)
       process.on('uncaughtException', () => true)
-      uncaughtExceptionMonitor.handleUncaughtException(error)
+      uncaughtExceptionMonitor.__listener(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)
+        uncaughtExceptionMonitor.__listener(error)
         expect(notifySpy).toHaveBeenCalledTimes(1)
       })
     })
 
     it('exits if it was already called and there are no other listeners', () => {
       uncaughtExceptionMonitor.__handlerAlreadyCalled = true
-      uncaughtExceptionMonitor.handleUncaughtException(error)
+      uncaughtExceptionMonitor.__listener(error)
       expect(notifySpy).not.toHaveBeenCalled()
       expect(fatallyLogAndExitSpy).toHaveBeenCalledWith(error)
     })
   })
 
   describe('hasOtherUncaughtExceptionListeners', () => {
     it('returns true if there are user-added listeners', () => {
-      process.on('uncaughtException', function honeybadgerUncaughtExceptionListener() {
-        return
-      })
+      uncaughtExceptionMonitor.maybeAddListener()
       process.on('uncaughtException', function domainUncaughtExceptionClear() {
         return 
       })
@@ -113,9 +149,7 @@ describe('UncaughtExceptionMonitor', () => {
     })
 
     it('returns false if there are only our expected listeners', () => {
-      process.on('uncaughtException', function honeybadgerUncaughtExceptionListener() {
-        return
-      })
+      uncaughtExceptionMonitor.maybeAddListener()
       process.on('uncaughtException', function domainUncaughtExceptionClear() {
         return 
       })

packages/js/test/unit/server/integrations/uncaught_exception_plugin.server.test.ts

@@ -3,6 +3,14 @@ import { TestTransport, TestClient, nullLogger } from '../../helpers'
 import * as util from '../../../../src/server/util'
 import Singleton from '../../../../src/server'
 
+function getListeners() {
+  return process.listeners('uncaughtException')
+}
+
+function getListenerCount() {
+  return getListeners().length
+}
+
 describe('Uncaught Exception Plugin', () => {
   let client: typeof Singleton
   let notifySpy: jest.SpyInstance
@@ -34,9 +42,9 @@ describe('Uncaught Exception Plugin', () => {
   describe('load', () => {
     const load = plugin().load
 
-    it('attaches a listener for uncaughtException', () => {
+    it('adds a listener for uncaughtException if enableUncaught is true', () => {
       load(client)
-      const listeners = process.listeners('uncaughtException')
+      const listeners = getListeners()
       expect(listeners.length).toBe(1)
       expect(listeners[0].name).toBe('honeybadgerUncaughtExceptionListener') 
 
@@ -45,11 +53,23 @@ describe('Uncaught Exception Plugin', () => {
       expect(notifySpy).toHaveBeenCalledTimes(1)
     })
 
-    it('returns if enableUncaught is not true', () => {
+    it('does not add a listener if enableUncaught is false', () => {
+      client.configure({ enableUncaught: false })
+      load(client)
+      expect(getListenerCount()).toBe(0)
+    })
+
+    it('adds or removes listener if needed when reloaded', () => {
+      load(client)
+      expect(getListenerCount()).toBe(1)
+
       client.configure({ enableUncaught: false })
       load(client)
-      const listeners = process.listeners('uncaughtException')
-      expect(listeners.length).toBe(0)
+      expect(getListenerCount()).toBe(0)
+
+      client.configure({ enableUncaught: true })
+      load(client)
+      expect(getListenerCount()).toBe(1)
     })
   })  
 })