ref: Leave only valid buffer implementation (#3744)

Author: kamilogorek

packages/utils/src/promisebuffer.ts

@@ -1,9 +1,6 @@
 import { SentryError } from './error';
-import { isThenable } from './is';
 import { SyncPromise } from './syncpromise';
 
-type TaskProducer<T> = () => PromiseLike<T>;
-
 /** A simple queue that holds promises. */
 export class PromiseBuffer<T> {
   /** Internal set of queued Promises */
@@ -21,22 +18,15 @@ export class PromiseBuffer<T> {
   /**
    * Add a promise to the queue.
    *
-   * @param taskProducer A function producing any PromiseLike<T>; In previous versions this used to be `@param task: PromiseLike<T>`, however, Promises were instantly created on the call-site, making them fall through the buffer limit.
+   * @param taskProducer A function producing any PromiseLike<T>; In previous versions this used to be `task: PromiseLike<T>`,
+   *        however, Promises were instantly created on the call-site, making them fall through the buffer limit.
    * @returns The original promise.
    */
-  public add(taskProducer: PromiseLike<T> | TaskProducer<T>): PromiseLike<T> {
-    // NOTE: This is necessary to preserve backwards compatibility
-    // It should accept _only_ `TaskProducer<T>` but we dont want to break other custom transports
-    // that are utilizing our `Buffer` implementation.
-    // see: https://github.com/getsentry/sentry-javascript/issues/3725
-    const normalizedTaskProducer: TaskProducer<T> = isThenable(taskProducer)
-      ? () => taskProducer as PromiseLike<T>
-      : (taskProducer as TaskProducer<T>);
-
+  public add(taskProducer: () => PromiseLike<T>): PromiseLike<T> {
     if (!this.isReady()) {
       return SyncPromise.reject(new SentryError('Not adding Promise due to buffer limit reached.'));
     }
-    const task = normalizedTaskProducer();
+    const task = taskProducer();
     if (this._buffer.indexOf(task) === -1) {
       this._buffer.push(task);
     }

packages/utils/test/promisebuffer.test.ts

@@ -1,117 +1,101 @@
-/* eslint-disable @typescript-eslint/no-floating-promises */
 import { PromiseBuffer } from '../src/promisebuffer';
 import { SyncPromise } from '../src/syncpromise';
 
 describe('PromiseBuffer', () => {
-  beforeEach(() => {
-    jest.useFakeTimers();
-  });
-
   describe('add()', () => {
     test('no limit', () => {
-      const q = new PromiseBuffer<void>();
-      const p = jest.fn(
-        () => new SyncPromise<void>(resolve => setTimeout(resolve, 1)),
-      );
-      q.add(p);
-      expect(q.length()).toBe(1);
+      const buffer = new PromiseBuffer();
+      const p = jest.fn(() => new SyncPromise(resolve => setTimeout(resolve)));
+      void buffer.add(p);
+      expect(buffer.length()).toEqual(1);
     });
 
     test('with limit', () => {
-      const q = new PromiseBuffer<void>(1);
-      let t1;
-      const p1 = jest.fn(() => {
-        t1 = new SyncPromise<void>(resolve => setTimeout(resolve, 1));
-        return t1;
+      const buffer = new PromiseBuffer(1);
+      let task1;
+      const producer1 = jest.fn(() => {
+        task1 = new SyncPromise(resolve => setTimeout(resolve));
+        return task1;
       });
-      const p2 = jest.fn(
-        () => new SyncPromise<void>(resolve => setTimeout(resolve, 1)),
-      );
-      expect(q.add(p1)).toEqual(t1);
-      expect(q.add(p2)).rejects.toThrowError();
-      expect(q.length()).toBe(1);
-      expect(p1).toHaveBeenCalled();
-      expect(p2).not.toHaveBeenCalled();
+      const producer2 = jest.fn(() => new SyncPromise(resolve => setTimeout(resolve)));
+      expect(buffer.add(producer1)).toEqual(task1);
+      void expect(buffer.add(producer2)).rejects.toThrowError();
+      expect(buffer.length()).toEqual(1);
+      expect(producer1).toHaveBeenCalled();
+      expect(producer2).not.toHaveBeenCalled();
     });
   });
 
-  test('resolved promises should not show up in buffer length', async () => {
-    expect.assertions(2);
-    const q = new PromiseBuffer<void>();
-    const p = new SyncPromise<void>(resolve => setTimeout(resolve, 1));
-    q.add(p).then(() => {
-      expect(q.length()).toBe(0);
+  describe('drain()', () => {
+    test('without timeout', async () => {
+      const buffer = new PromiseBuffer();
+      for (let i = 0; i < 5; i++) {
+        void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve)));
+      }
+      expect(buffer.length()).toEqual(5);
+      const result = await buffer.drain();
+      expect(result).toEqual(true);
+      expect(buffer.length()).toEqual(0);
     });
-    expect(q.length()).toBe(1);
-    jest.runAllTimers();
-  });
 
-  test('receive promise result outside and from buffer', async () => {
-    expect.assertions(4);
-    const q = new PromiseBuffer<string>();
-    const p = new SyncPromise<string>(resolve =>
-      setTimeout(() => {
-        resolve('test');
-      }, 1),
-    );
-    q.add(p).then(result => {
-      expect(q.length()).toBe(0);
-      expect(result).toBe('test');
+    test('with timeout', async () => {
+      const buffer = new PromiseBuffer();
+      for (let i = 0; i < 5; i++) {
+        void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve, 100)));
+      }
+      expect(buffer.length()).toEqual(5);
+      const result = await buffer.drain(50);
+      expect(result).toEqual(false);
     });
-    expect(q.length()).toBe(1);
-    p.then(result => {
-      expect(result).toBe('test');
+
+    test('on empty buffer', async () => {
+      const buffer = new PromiseBuffer();
+      expect(buffer.length()).toEqual(0);
+      const result = await buffer.drain();
+      expect(result).toEqual(true);
+      expect(buffer.length()).toEqual(0);
     });
-    jest.runAllTimers();
   });
 
-  test('drain()', async () => {
-    expect.assertions(3);
-    const q = new PromiseBuffer<void>();
-    for (let i = 0; i < 5; i++) {
-      const p = new SyncPromise<void>(resolve => setTimeout(resolve, 1));
-      q.add(p);
-    }
-    expect(q.length()).toBe(5);
-    q.drain().then(result => {
-      expect(result).toBeTruthy();
-      expect(q.length()).toBe(0);
-    });
-    jest.runAllTimers();
+  test('resolved promises should not show up in buffer length', async () => {
+    const buffer = new PromiseBuffer();
+    const producer = () => new SyncPromise(resolve => setTimeout(resolve));
+    const task = buffer.add(producer);
+    expect(buffer.length()).toEqual(1);
+    await task;
+    expect(buffer.length()).toEqual(0);
   });
 
-  test('drain() with timeout', async () => {
-    expect.assertions(2);
-    const q = new PromiseBuffer<void>();
-    for (let i = 0; i < 5; i++) {
-      const p = new SyncPromise<void>(resolve => setTimeout(resolve, 100));
-      q.add(p);
+  test('rejected promises should not show up in buffer length', async () => {
+    const buffer = new PromiseBuffer();
+    const producer = () => new SyncPromise((_, reject) => setTimeout(reject));
+    const task = buffer.add(producer);
+    expect(buffer.length()).toEqual(1);
+    try {
+      await task;
+    } catch (_) {
+      // no-empty
     }
-    expect(q.length()).toBe(5);
-    q.drain(50).then(result => {
-      expect(result).toBeFalsy();
-    });
-    jest.runAllTimers();
+    expect(buffer.length()).toEqual(0);
   });
 
-  test('drain() on empty buffer', async () => {
-    expect.assertions(3);
-    const q = new PromiseBuffer<void>();
-    expect(q.length()).toBe(0);
-    q.drain().then(result => {
-      expect(result).toBeTruthy();
-      expect(q.length()).toBe(0);
-    });
-    jest.runAllTimers();
+  test('resolved task should give an access to the return value', async () => {
+    const buffer = new PromiseBuffer<string>();
+    const producer = () => new SyncPromise<string>(resolve => setTimeout(() => resolve('test')));
+    const task = buffer.add(producer);
+    const result = await task;
+    expect(result).toEqual('test');
   });
 
-  test('rejecting', async () => {
+  test('rejected task should give an access to the return value', async () => {
     expect.assertions(1);
-    const q = new PromiseBuffer<void>();
-    const p = new SyncPromise<void>((_, reject) => setTimeout(reject, 1));
-    jest.runAllTimers();
-    return q.add(p).then(null, () => {
-      expect(true).toBe(true);
-    });
+    const buffer = new PromiseBuffer<string>();
+    const producer = () => new SyncPromise<string>((_, reject) => setTimeout(() => reject(new Error('whoops'))));
+    const task = buffer.add(producer);
+    try {
+      await task;
+    } catch (e) {
+      expect(e).toEqual(new Error('whoops'));
+    }
   });
 });