ref(core): Streamline event processor handling (#17634)

Avoid re-creating many sync promises where not needed.

We used to wrap every result of an event processor in a sync promise, no
matter if needed or not. This is not really necessary, likely having a
(very minor) perf impact but also making the stacktraces etc. more
complicated then neccessary.

With this PR, we only wrap the overall result in a sync promise once, if
needed, and otherwise just run the event processors directly.

Somewhat inspired by looking into this for
#17592, but likely
does not actually fix this.

Author: mydea

packages/core/src/eventProcessors.ts

@@ -3,7 +3,7 @@ import type { Event, EventHint } from './types-hoist/event';
 import type { EventProcessor } from './types-hoist/eventprocessor';
 import { debug } from './utils/debug-logger';
 import { isThenable } from './utils/is';
-import { SyncPromise } from './utils/syncpromise';
+import { rejectedSyncPromise, resolvedSyncPromise } from './utils/syncpromise';
 
 /**
  * Process an array of event processors, returning the processed event (or `null` if the event was dropped).
@@ -14,24 +14,33 @@ export function notifyEventProcessors(
   hint: EventHint,
   index: number = 0,
 ): PromiseLike<Event | null> {
-  return new SyncPromise<Event | null>((resolve, reject) => {
-    const processor = processors[index];
-    if (event === null || typeof processor !== 'function') {
-      resolve(event);
-    } else {
-      const result = processor({ ...event }, hint) as Event | null;
+  try {
+    const result = _notifyEventProcessors(event, hint, processors, index);
+    return isThenable(result) ? result : resolvedSyncPromise(result);
+  } catch (error) {
+    return rejectedSyncPromise(error);
+  }
+}
+
+function _notifyEventProcessors(
+  event: Event | null,
+  hint: EventHint,
+  processors: EventProcessor[],
+  index: number,
+): Event | null | PromiseLike<Event | null> {
+  const processor = processors[index];
+
+  if (!event || !processor) {
+    return event;
+  }
+
+  const result = processor({ ...event }, hint);
+
+  DEBUG_BUILD && result === null && debug.log(`Event processor "${processor.id || '?'}" dropped event`);
 
-      DEBUG_BUILD && processor.id && result === null && debug.log(`Event processor "${processor.id}" dropped event`);
+  if (isThenable(result)) {
+    return result.then(final => _notifyEventProcessors(final, hint, processors, index + 1));
+  }
 
-      if (isThenable(result)) {
-        void result
-          .then(final => notifyEventProcessors(processors, final, hint, index + 1).then(resolve))
-          .then(null, reject);
-      } else {
-        void notifyEventProcessors(processors, result, hint, index + 1)
-          .then(resolve)
-          .then(null, reject);
-      }
-    }
-  });
+  return _notifyEventProcessors(result, hint, processors, index + 1);
 }

packages/core/test/lib/client.test.ts

@@ -1833,15 +1833,13 @@ describe('Client', () => {
       });
     });
 
-    test('event processor sends an event and logs when it crashes', () => {
-      expect.assertions(3);
-
+    test('event processor sends an event and logs when it crashes synchronously', () => {
       const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
       const client = new TestClient(options);
       const captureExceptionSpy = vi.spyOn(client, 'captureException');
       const loggerWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn');
       const scope = new Scope();
-      const exception = new Error('sorry');
+      const exception = new Error('sorry 1');
       scope.addEventProcessor(() => {
         throw exception;
       });
@@ -1850,7 +1848,43 @@ describe('Client', () => {
 
       expect(TestClient.instance!.event!.exception!.values![0]).toStrictEqual({
         type: 'Error',
-        value: 'sorry',
+        value: 'sorry 1',
+        mechanism: { type: 'internal', handled: false },
+      });
+      expect(captureExceptionSpy).toBeCalledWith(exception, {
+        data: {
+          __sentry__: true,
+        },
+        originalException: exception,
+        mechanism: { type: 'internal', handled: false },
+      });
+      expect(loggerWarnSpy).toBeCalledWith(
+        `Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${exception}`,
+      );
+    });
+
+    test('event processor sends an event and logs when it crashes asynchronously', async () => {
+      vi.useFakeTimers();
+
+      const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
+      const client = new TestClient(options);
+      const captureExceptionSpy = vi.spyOn(client, 'captureException');
+      const loggerWarnSpy = vi.spyOn(debugLoggerModule.debug, 'warn');
+      const scope = new Scope();
+      const exception = new Error('sorry 2');
+      scope.addEventProcessor(() => {
+        return new Promise((_resolve, reject) => {
+          reject(exception);
+        });
+      });
+
+      client.captureEvent({ message: 'hello' }, {}, scope);
+
+      await vi.runOnlyPendingTimersAsync();
+
+      expect(TestClient.instance!.event!.exception!.values![0]).toStrictEqual({
+        type: 'Error',
+        value: 'sorry 2',
         mechanism: { type: 'internal', handled: false },
       });
       expect(captureExceptionSpy).toBeCalledWith(exception, {
@@ -1865,6 +1899,81 @@ describe('Client', () => {
       );
     });
 
+    test('event processor sends an event and logs when it crashes synchronously in processor chain', () => {
+      const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
+      const client = new TestClient(options);
+      const captureExceptionSpy = vi.spyOn(client, 'captureException');
+      const scope = new Scope();
+      const exception = new Error('sorry 3');
+
+      const processor1 = vi.fn(event => {
+        return event;
+      });
+      const processor2 = vi.fn(() => {
+        throw exception;
+      });
+      const processor3 = vi.fn(event => {
+        return event;
+      });
+
+      scope.addEventProcessor(processor1);
+      scope.addEventProcessor(processor2);
+      scope.addEventProcessor(processor3);
+
+      client.captureEvent({ message: 'hello' }, {}, scope);
+
+      expect(processor1).toHaveBeenCalledTimes(1);
+      expect(processor2).toHaveBeenCalledTimes(1);
+      expect(processor3).toHaveBeenCalledTimes(0);
+
+      expect(captureExceptionSpy).toBeCalledWith(exception, {
+        data: {
+          __sentry__: true,
+        },
+        originalException: exception,
+        mechanism: { type: 'internal', handled: false },
+      });
+    });
+
+    test('event processor sends an event and logs when it crashes asynchronously in processor chain', async () => {
+      vi.useFakeTimers();
+
+      const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
+      const client = new TestClient(options);
+      const captureExceptionSpy = vi.spyOn(client, 'captureException');
+      const scope = new Scope();
+      const exception = new Error('sorry 4');
+
+      const processor1 = vi.fn(async event => {
+        return event;
+      });
+      const processor2 = vi.fn(async () => {
+        throw exception;
+      });
+      const processor3 = vi.fn(event => {
+        return event;
+      });
+
+      scope.addEventProcessor(processor1);
+      scope.addEventProcessor(processor2);
+      scope.addEventProcessor(processor3);
+
+      client.captureEvent({ message: 'hello' }, {}, scope);
+      await vi.runOnlyPendingTimersAsync();
+
+      expect(processor1).toHaveBeenCalledTimes(1);
+      expect(processor2).toHaveBeenCalledTimes(1);
+      expect(processor3).toHaveBeenCalledTimes(0);
+
+      expect(captureExceptionSpy).toBeCalledWith(exception, {
+        data: {
+          __sentry__: true,
+        },
+        originalException: exception,
+        mechanism: { type: 'internal', handled: false },
+      });
+    });
+
     test('records events dropped due to `sampleRate` option', () => {
       expect.assertions(1);