Centralize attached handler cleanup

Author: eli-sterling

src/cleanup/attached-handlers.mjs

@@ -0,0 +1,29 @@
+import { removeProcessListener } from "./listener.mjs";
+
+export function removeAttachedHandlers({
+  processObj,
+  remove,
+  events,
+  handlers,
+  shouldSkip,
+  markRemoved,
+}) {
+  let removalError;
+  let hasRemovalError = false;
+  for (const event of events) {
+    if (shouldSkip(event)) {
+      markRemoved(event);
+      continue;
+    }
+    try {
+      removeProcessListener({ processObj, remove, event, handler: handlers[event] });
+      markRemoved(event);
+    } catch (error) {
+      if (!hasRemovalError) {
+        removalError = error;
+        hasRemovalError = true;
+      }
+    }
+  }
+  return { removalError, hasRemovalError };
+}

src/handler-removal.mjs

@@ -1,26 +1,16 @@
-import { removeProcessListener } from "./cleanup/listener.mjs";
+import { removeAttachedHandlers } from "./cleanup/attached-handlers.mjs";
 
 export function createHandlerRemoval({ processObj, remove, selected, handlers, detachedEvents }) {
   const removedEvents = new Set();
   return () => {
-    let removalError;
-    let hasRemovalError = false;
-    for (const event of selected) {
-      if (removedEvents.has(event)) continue;
-      if (detachedEvents.has(event)) {
-        removedEvents.add(event);
-        continue;
-      }
-      try {
-        removeProcessListener({ processObj, remove, event, handler: handlers[event] });
-        removedEvents.add(event);
-      } catch (error) {
-        if (!hasRemovalError) {
-          removalError = error;
-          hasRemovalError = true;
-        }
-      }
-    }
+    const { removalError, hasRemovalError } = removeAttachedHandlers({
+      processObj,
+      remove,
+      events: selected,
+      handlers,
+      shouldSkip: (event) => removedEvents.has(event) || detachedEvents.has(event),
+      markRemoved: (event) => removedEvents.add(event),
+    });
     if (hasRemovalError) throw removalError;
   };
 }

src/setup/rollback.mjs

@@ -1,5 +1,5 @@
 import { attachFailure } from "../failure.mjs";
-import { removeProcessListener } from "../cleanup/listener.mjs";
+import { removeAttachedHandlers } from "../cleanup/attached-handlers.mjs";
 
 export function rollbackRegistration({
   error,
@@ -10,18 +10,14 @@ export function rollbackRegistration({
   registration,
   registrations,
 }) {
-  let rollbackError;
-  let rollbackFailed = false;
-  for (const event of attachedEvents) {
-    try {
-      removeProcessListener({ processObj, remove, event, handler: handlers[event] });
-    } catch (removalError) {
-      if (!rollbackFailed) {
-        rollbackError = removalError;
-        rollbackFailed = true;
-      }
-    }
-  }
+  const { removalError: rollbackError, hasRemovalError: rollbackFailed } = removeAttachedHandlers({
+    processObj,
+    remove,
+    events: attachedEvents,
+    handlers,
+    shouldSkip: () => false,
+    markRemoved: () => {},
+  });
   if (rollbackFailed) {
     registration.ready = true;
     registrations.set(processObj, registration);

tests/cleanup/attached-handlers.test.mjs

@@ -0,0 +1,46 @@
+import { jest } from "@jest/globals";
+import { removeAttachedHandlers } from "../../src/cleanup/attached-handlers.mjs";
+
+test("skips selected handlers and records removed events", () => {
+  const handler = jest.fn();
+  const processObj = { off: jest.fn() };
+  const removedEvents = new Set();
+  const result = removeAttachedHandlers({
+    processObj,
+    remove: "off",
+    events: ["warning", "uncaughtException"],
+    handlers: { warning: handler, uncaughtException: jest.fn() },
+    shouldSkip: (event) => event === "uncaughtException",
+    markRemoved: (event) => removedEvents.add(event),
+  });
+
+  expect(result).toEqual({ removalError: undefined, hasRemovalError: false });
+  expect(processObj.off).toHaveBeenCalledTimes(1);
+  expect(processObj.off).toHaveBeenCalledWith("warning", handler);
+  expect(removedEvents).toEqual(new Set(["warning", "uncaughtException"]));
+});
+
+test("attempts all events and returns the first failure", () => {
+  const first = Error("first removal");
+  const processObj = {
+    off: jest
+      .fn()
+      .mockImplementationOnce(() => {
+        throw first;
+      })
+      .mockImplementationOnce(() => {
+        throw Error("second removal");
+      }),
+  };
+  const result = removeAttachedHandlers({
+    processObj,
+    remove: "off",
+    events: ["warning", "uncaughtException"],
+    handlers: { warning: jest.fn(), uncaughtException: jest.fn() },
+    shouldSkip: () => false,
+    markRemoved: jest.fn(),
+  });
+
+  expect(result).toEqual({ removalError: first, hasRemovalError: true });
+  expect(processObj.off).toHaveBeenCalledTimes(2);
+});

tests/event/handler.test.mjs

@@ -31,25 +31,6 @@ test("logs events and ignores removed or fired handlers", () => {
   expect(handler("ignored")).toBeUndefined();
 });
 
-test("propagates logger failures from warning and rejection callbacks", () => {
-  const processObj = { on: jest.fn(), off: jest.fn() };
-  const log = {
-    error: jest.fn(() => {
-      throw Error("rejection log");
-    }),
-    warn: jest.fn(() => {
-      throw Error("warning log");
-    }),
-    debug: jest.fn(),
-  };
-  registerHandlers({ processObj, log, events: ["warning", "unhandledRejection"] });
-  const handlers = Object.fromEntries(
-    processObj.on.mock.calls.map(([event, handler]) => [event, handler]),
-  );
-  expect(() => handlers.warning("warning")).toThrow("warning log");
-  expect(() => handlers.unhandledRejection("reason")).toThrow("rejection log");
-});
-
 test("propagates synchronous one-shot logger failures during setup", () => {
   const processObj = {
     on: jest.fn(),

tests/logging.test.mjs

@@ -27,6 +27,26 @@ test("routes warning and rejection events to the logger", () => {
   expect(log.error).toHaveBeenCalledWith("Unhandled promise error", { reasonType: "string" });
 });
 
+test("propagates logger failures from warning and rejection callbacks", () => {
+  const processObj = { on: jest.fn(), off: jest.fn() };
+  const log = {
+    error: jest.fn(() => {
+      throw Error("rejection log");
+    }),
+    warn: jest.fn(() => {
+      throw Error("warning log");
+    }),
+    debug: jest.fn(),
+  };
+  registerHandlers({ processObj, log, events: ["warning", "unhandledRejection"] });
+  const handlers = Object.fromEntries(
+    processObj.on.mock.calls.map(([event, handler]) => [event, handler]),
+  );
+
+  expect(() => handlers.warning("warning")).toThrow("warning log");
+  expect(() => handlers.unhandledRejection("reason")).toThrow("rejection log");
+});
+
 test("logs uncaught exceptions", () => {
   const processObj = { on: jest.fn(), off: jest.fn() };
   const log = { debug: jest.fn(), error: jest.fn(), warn: jest.fn() };