From 29b69143c3630201b8fada44442900ec5464542a Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 16 Dec 2021 14:12:01 +0100 Subject: [PATCH 1/5] ref(utils): refactor promisebuffer --- packages/browser/src/transports/base.ts | 3 +- packages/core/test/mocks/transport.ts | 4 +- packages/node/src/transports/base/index.ts | 11 ++- packages/utils/src/promisebuffer.ts | 86 +++++++++++++--------- packages/utils/test/promisebuffer.test.ts | 20 ++--- 5 files changed, 74 insertions(+), 50 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 0b4fb9faa195..042311d53694 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -17,6 +17,7 @@ import { eventStatusFromHttpCode, getGlobalObject, logger, + makePromiseBuffer, parseRetryAfterHeader, PromiseBuffer, SentryError, @@ -42,7 +43,7 @@ export abstract class BaseTransport implements Transport { protected readonly _api: APIDetails; /** A simple buffer holding all requests. */ - protected readonly _buffer: PromiseBuffer = new PromiseBuffer(30); + protected readonly _buffer: PromiseBuffer = makePromiseBuffer(30); /** Locks transport after receiving rate limits in a response */ protected readonly _rateLimits: Record = {}; diff --git a/packages/core/test/mocks/transport.ts b/packages/core/test/mocks/transport.ts index 29c3be68d226..360667ef51da 100644 --- a/packages/core/test/mocks/transport.ts +++ b/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 { 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 = new PromiseBuffer(9999); + protected readonly _buffer: PromiseBuffer = makePromiseBuffer(9999); public sendEvent(_event: Event): PromiseLike { this.sendCalled += 1; diff --git a/packages/node/src/transports/base/index.ts b/packages/node/src/transports/base/index.ts index 42376ec653e5..f69be6c2f1b1 100644 --- a/packages/node/src/transports/base/index.ts +++ b/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 = new PromiseBuffer(30); + protected readonly _buffer: PromiseBuffer = makePromiseBuffer(30); /** Locks transport after receiving rate limits in a response */ protected readonly _rateLimits: Record = {}; diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index 94c4a8f48d00..3753b99198a4 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -22,18 +22,41 @@ function allPromises(collection: Array>): Promis }); } -/** A simple queue that holds promises. */ -export class PromiseBuffer { - /** Internal set of queued Promises */ - private readonly _buffer: Array> = []; +export interface PromiseBuffer { + _buffer: Array>; + isReady(): boolean; + add(taskProducer: () => PromiseLike): PromiseLike; + remove(task: PromiseLike): PromiseLike; + length(): number; + drain(timeout?: number): PromiseLike; +} + +/** + * 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(limit?: number): PromiseBuffer { + const buffer: Array> = []; + + /** + * This function returns the number of unresolved promises in the queue. + */ + function length(): number { + return buffer.length; + } - public constructor(protected _limit?: number) {} + function isReady(): boolean { + return limit === undefined || length() < limit; + } /** - * Says if the buffer is ready to take more requests + * Remove a promise from the queue. + * + * @param task Can be any PromiseLike + * @returns Removed promise. */ - public isReady(): boolean { - return this._limit === undefined || this.length() < this._limit; + function remove(task: PromiseLike): PromiseLike { + return buffer.splice(buffer.indexOf(task), 1)[0]; } /** @@ -46,47 +69,29 @@ export class PromiseBuffer { * limit check. * @returns The original promise. */ - public add(taskProducer: () => PromiseLike): PromiseLike { - if (!this.isReady()) { + function add(taskProducer: () => PromiseLike): PromiseLike { + if (!isReady()) { return SyncPromise.reject(new SentryError('Not adding Promise due to buffer limit reached.')); } // start the task and add its promise to the queue const task = taskProducer(); - if (this._buffer.indexOf(task) === -1) { - this._buffer.push(task); + if (buffer.indexOf(task) === -1) { + buffer.push(task); } void task - .then(() => this.remove(task)) + .then(() => remove(task)) // Use `then(null, rejectionHandler)` rather than `catch(rejectionHandler)` so that we can use `PromiseLike` // rather than `Promise`. `PromiseLike` doesn't have a `.catch` method, making its polyfill smaller. (ES5 didn't // have promises, so TS has to polyfill when down-compiling.) .then(null, () => - this.remove(task).then(null, () => { - // We have to add another catch here because `this.remove()` starts a new promise chain. + remove(task).then(null, () => { + // We have to add another catch here because `remove()` starts a new promise chain. }), ); return task; } - /** - * Remove a promise from the queue. - * - * @param task Can be any PromiseLike - * @returns Removed promise. - */ - public remove(task: PromiseLike): PromiseLike { - const removedTask = this._buffer.splice(this._buffer.indexOf(task), 1)[0]; - return removedTask; - } - - /** - * This function returns the number of unresolved promises in the queue. - */ - public length(): number { - return this._buffer.length; - } - /** * Wait for all promises in the queue to resolve or for timeout to expire, whichever comes first. * @@ -96,7 +101,7 @@ export class PromiseBuffer { * @returns A promise which will resolve to `true` if the queue is already empty or drains before the timeout, and * `false` otherwise */ - public drain(timeout?: number): PromiseLike { + function drain(timeout?: number): PromiseLike { return new SyncPromise(resolve => { // wait for `timeout` ms and then resolve to `false` (if not cancelled first) const capturedSetTimeout = setTimeout(() => { @@ -106,10 +111,21 @@ export class PromiseBuffer { }, timeout); // if all promises resolve in time, cancel the timer and resolve to `true` - void allPromises(this._buffer).then(() => { + void allPromises(buffer).then(() => { clearTimeout(capturedSetTimeout); resolve(true); }); }); } + + const promiseBuffer: PromiseBuffer = { + _buffer: buffer, + length, + isReady, + add, + remove, + drain, + }; + + return promiseBuffer; } diff --git a/packages/utils/test/promisebuffer.test.ts b/packages/utils/test/promisebuffer.test.ts index 47b3cc8a5444..7e9930ecc55b 100644 --- a/packages/utils/test/promisebuffer.test.ts +++ b/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(); + const buffer = makePromiseBuffer(); const producer = () => new SyncPromise(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(); + const buffer = makePromiseBuffer(); const producer = () => new SyncPromise((_, reject) => setTimeout(() => reject(new Error('whoops')))); const task = buffer.add(producer); try { From e126b58051db9013d2d7a5899d3ac14f0b181e56 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 16 Dec 2021 16:09:07 +0100 Subject: [PATCH 2/5] ref(utils): remove length and isReady from public api --- packages/utils/src/promisebuffer.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index 3753b99198a4..d26384ccc81d 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -24,10 +24,8 @@ function allPromises(collection: Array>): Promis export interface PromiseBuffer { _buffer: Array>; - isReady(): boolean; add(taskProducer: () => PromiseLike): PromiseLike; remove(task: PromiseLike): PromiseLike; - length(): number; drain(timeout?: number): PromiseLike; } @@ -38,15 +36,8 @@ export interface PromiseBuffer { export function makePromiseBuffer(limit?: number): PromiseBuffer { const buffer: Array> = []; - /** - * This function returns the number of unresolved promises in the queue. - */ - function length(): number { - return buffer.length; - } - function isReady(): boolean { - return limit === undefined || length() < limit; + return limit === undefined || buffer.length < limit; } /** @@ -120,8 +111,6 @@ export function makePromiseBuffer(limit?: number): PromiseBuffer { const promiseBuffer: PromiseBuffer = { _buffer: buffer, - length, - isReady, add, remove, drain, From 4a7463ff276f966a3cd6ab785aa33a5104627487 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 16 Dec 2021 17:47:12 +0100 Subject: [PATCH 3/5] ref(promisebuffer): remove _buffer --- packages/node/src/transports/base/index.ts | 2 +- packages/utils/src/promisebuffer.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/node/src/transports/base/index.ts b/packages/node/src/transports/base/index.ts index f69be6c2f1b1..6225bed07c14 100644 --- a/packages/node/src/transports/base/index.ts +++ b/packages/node/src/transports/base/index.ts @@ -13,9 +13,9 @@ import { import { eventStatusFromHttpCode, logger, + makePromiseBuffer, parseRetryAfterHeader, PromiseBuffer, - makePromiseBuffer, SentryError, } from '@sentry/utils'; import * as fs from 'fs'; diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index d26384ccc81d..df6b49ae329c 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -23,7 +23,6 @@ function allPromises(collection: Array>): Promis } export interface PromiseBuffer { - _buffer: Array>; add(taskProducer: () => PromiseLike): PromiseLike; remove(task: PromiseLike): PromiseLike; drain(timeout?: number): PromiseLike; @@ -110,7 +109,6 @@ export function makePromiseBuffer(limit?: number): PromiseBuffer { } const promiseBuffer: PromiseBuffer = { - _buffer: buffer, add, remove, drain, From e510c25c2533c673b9f12c186f262272b5e617a4 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 16 Dec 2021 18:23:16 +0100 Subject: [PATCH 4/5] fix(lint): run linter --- packages/core/test/mocks/transport.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/mocks/transport.ts b/packages/core/test/mocks/transport.ts index 360667ef51da..1037fada987d 100644 --- a/packages/core/test/mocks/transport.ts +++ b/packages/core/test/mocks/transport.ts @@ -1,5 +1,5 @@ import { Event, Response, Transport } from '@sentry/types'; -import { PromiseBuffer, makePromiseBuffer, SyncPromise } from '@sentry/utils'; +import { makePromiseBuffer, PromiseBuffer, SyncPromise } from '@sentry/utils'; async function sleep(delay: number): Promise { return new SyncPromise(resolve => setTimeout(resolve, delay)); From cbc37592cc924e5f94b7e993397ab9a790053deb Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 16 Dec 2021 19:13:45 +0100 Subject: [PATCH 5/5] fix(promisebuffer): add back the promise buffer --- packages/utils/src/promisebuffer.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index df6b49ae329c..5aee7b449135 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -23,6 +23,7 @@ function allPromises(collection: Array>): Promis } export interface PromiseBuffer { + length(): number; add(taskProducer: () => PromiseLike): PromiseLike; remove(task: PromiseLike): PromiseLike; drain(timeout?: number): PromiseLike; @@ -35,6 +36,10 @@ export interface PromiseBuffer { export function makePromiseBuffer(limit?: number): PromiseBuffer { const buffer: Array> = []; + function length(): number { + return buffer.length; + } + function isReady(): boolean { return limit === undefined || buffer.length < limit; } @@ -109,6 +114,7 @@ export function makePromiseBuffer(limit?: number): PromiseBuffer { } const promiseBuffer: PromiseBuffer = { + length, add, remove, drain,