ref(utils): refactor promisebuffer

Author: JonasBa

packages/browser/src/transports/base.ts

@@ -17,8 +17,9 @@ import {
   eventStatusFromHttpCode,
   getGlobalObject,
   logger,
-  parseRetryAfterHeader,
   makePromiseBuffer,
+  parseRetryAfterHeader,
+  PromiseBuffer,
   SentryError,
 } from '@sentry/utils';
 
@@ -42,7 +43,7 @@ export abstract class BaseTransport implements Transport {
   protected readonly _api: APIDetails;
 
   /** A simple buffer holding all requests. */
-  protected readonly _buffer: PromiseBuffer<SentryResponse> = new PromiseBuffer(30);
+  protected readonly _buffer: PromiseBuffer<SentryResponse> = makePromiseBuffer(30);
 
   /** Locks transport after receiving rate limits in a response */
   protected readonly _rateLimits: Record<string, Date> = {};

packages/core/test/mocks/transport.ts

@@ -1,5 +1,5 @@
 import { Event, Response, Transport } from '@sentry/types';
-import { PromiseBuffer, SyncPromise } from '@sentry/utils';
+import { PromiseBuffer, makePromiseBuffer, SyncPromise } from '@sentry/utils';
 
 async function sleep(delay: number): Promise<void> {
   return new SyncPromise(resolve => setTimeout(resolve, delay));
@@ -11,7 +11,7 @@ export class FakeTransport implements Transport {
   public delay: number = 2000;
 
   /** A simple buffer holding all requests. */
-  protected readonly _buffer: PromiseBuffer<Response> = new PromiseBuffer(9999);
+  protected readonly _buffer: PromiseBuffer<Response> = makePromiseBuffer(9999);
 
   public sendEvent(_event: Event): PromiseLike<Response> {
     this.sendCalled += 1;

packages/node/src/transports/base/index.ts

@@ -10,7 +10,14 @@ import {
   Transport,
   TransportOptions,
 } from '@sentry/types';
-import { eventStatusFromHttpCode, logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils';
+import {
+  eventStatusFromHttpCode,
+  logger,
+  parseRetryAfterHeader,
+  PromiseBuffer,
+  makePromiseBuffer,
+  SentryError,
+} from '@sentry/utils';
 import * as fs from 'fs';
 import * as http from 'http';
 import * as https from 'https';
@@ -43,7 +50,7 @@ export abstract class BaseTransport implements Transport {
   protected _api: APIDetails;
 
   /** A simple buffer holding all requests. */
-  protected readonly _buffer: PromiseBuffer<Response> = new PromiseBuffer(30);
+  protected readonly _buffer: PromiseBuffer<Response> = makePromiseBuffer(30);
 
   /** Locks transport after receiving rate limits in a response */
   protected readonly _rateLimits: Record<string, Date> = {};

packages/utils/src/promisebuffer.ts

@@ -22,7 +22,7 @@ function allPromises<U = unknown>(collection: Array<U | PromiseLike<U>>): Promis
   });
 }
 
-interface PromiseBuffer<T> {
+export interface PromiseBuffer<T> {
   _buffer: Array<PromiseLike<T>>;
   isReady(): boolean;
   add(taskProducer: () => PromiseLike<T>): PromiseLike<T>;
@@ -31,13 +31,11 @@ interface PromiseBuffer<T> {
   drain(timeout?: number): PromiseLike<boolean>;
 }
 
-const DEFAULT_BUFFER_LIMIT = 30;
-
 /**
- * Create a new promise buffer
- * @param limit
+ * Creates an new PromiseBuffer object with the specified limit
+ * @param limit max number of promises that can be stored in the buffer
  */
-export function makePromiseBuffer<T>(limit: number = DEFAULT_BUFFER_LIMIT): PromiseBuffer<T> {
+export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
   const buffer: Array<PromiseLike<T>> = [];
 
   /**
@@ -48,7 +46,7 @@ export function makePromiseBuffer<T>(limit: number = DEFAULT_BUFFER_LIMIT): Prom
   }
 
   function isReady(): boolean {
-    return length() < limit;
+    return limit === undefined || length() < limit;
   }
 
   /**
@@ -88,7 +86,7 @@ export function makePromiseBuffer<T>(limit: number = DEFAULT_BUFFER_LIMIT): Prom
       // have promises, so TS has to polyfill when down-compiling.)
       .then(null, () =>
         remove(task).then(null, () => {
-          // We have to add another catch here because `this.remove()` starts a new promise chain.
+          // We have to add another catch here because `remove()` starts a new promise chain.
         }),
       );
     return task;

packages/utils/test/promisebuffer.test.ts

@@ -1,17 +1,17 @@
-import { PromiseBuffer } from '../src/promisebuffer';
+import { makePromiseBuffer } from '../src/promisebuffer';
 import { SyncPromise } from '../src/syncpromise';
 
 describe('PromiseBuffer', () => {
   describe('add()', () => {
     test('no limit', () => {
-      const buffer = new PromiseBuffer();
+      const buffer = makePromiseBuffer();
       const p = jest.fn(() => new SyncPromise(resolve => setTimeout(resolve)));
       void buffer.add(p);
       expect(buffer.length()).toEqual(1);
     });
 
     test('with limit', () => {
-      const buffer = new PromiseBuffer(1);
+      const buffer = makePromiseBuffer(1);
       let task1;
       const producer1 = jest.fn(() => {
         task1 = new SyncPromise(resolve => setTimeout(resolve));
@@ -28,7 +28,7 @@ describe('PromiseBuffer', () => {
 
   describe('drain()', () => {
     test('without timeout', async () => {
-      const buffer = new PromiseBuffer();
+      const buffer = makePromiseBuffer();
       for (let i = 0; i < 5; i++) {
         void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve)));
       }
@@ -39,7 +39,7 @@ describe('PromiseBuffer', () => {
     });
 
     test('with timeout', async () => {
-      const buffer = new PromiseBuffer();
+      const buffer = makePromiseBuffer();
       for (let i = 0; i < 5; i++) {
         void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve, 100)));
       }
@@ -49,7 +49,7 @@ describe('PromiseBuffer', () => {
     });
 
     test('on empty buffer', async () => {
-      const buffer = new PromiseBuffer();
+      const buffer = makePromiseBuffer();
       expect(buffer.length()).toEqual(0);
       const result = await buffer.drain();
       expect(result).toEqual(true);
@@ -58,7 +58,7 @@ describe('PromiseBuffer', () => {
   });
 
   test('resolved promises should not show up in buffer length', async () => {
-    const buffer = new PromiseBuffer();
+    const buffer = makePromiseBuffer();
     const producer = () => new SyncPromise(resolve => setTimeout(resolve));
     const task = buffer.add(producer);
     expect(buffer.length()).toEqual(1);
@@ -67,7 +67,7 @@ describe('PromiseBuffer', () => {
   });
 
   test('rejected promises should not show up in buffer length', async () => {
-    const buffer = new PromiseBuffer();
+    const buffer = makePromiseBuffer();
     const producer = () => new SyncPromise((_, reject) => setTimeout(reject));
     const task = buffer.add(producer);
     expect(buffer.length()).toEqual(1);
@@ -80,7 +80,7 @@ describe('PromiseBuffer', () => {
   });
 
   test('resolved task should give an access to the return value', async () => {
-    const buffer = new PromiseBuffer<string>();
+    const buffer = makePromiseBuffer<string>();
     const producer = () => new SyncPromise<string>(resolve => setTimeout(() => resolve('test')));
     const task = buffer.add(producer);
     const result = await task;
@@ -89,7 +89,7 @@ describe('PromiseBuffer', () => {
 
   test('rejected task should give an access to the return value', async () => {
     expect.assertions(1);
-    const buffer = new PromiseBuffer<string>();
+    const buffer = makePromiseBuffer<string>();
     const producer = () => new SyncPromise<string>((_, reject) => setTimeout(() => reject(new Error('whoops'))));
     const task = buffer.add(producer);
     try {