From 61eda62ed5df5654f93e34a4848fc9ae3fcac0f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Fri, 25 Jun 2021 12:15:46 +0200 Subject: [PATCH] ref: Leave only valid buffer implementation (#3744) --- packages/utils/src/promisebuffer.ts | 18 +-- packages/utils/test/promisebuffer.test.ts | 160 ++++++++++------------ 2 files changed, 76 insertions(+), 102 deletions(-) diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index 033c40d18c51..b27d039cf0b2 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -1,9 +1,6 @@ import { SentryError } from './error'; -import { isThenable } from './is'; import { SyncPromise } from './syncpromise'; -type TaskProducer = () => PromiseLike; - /** A simple queue that holds promises. */ export class PromiseBuffer { /** Internal set of queued Promises */ @@ -21,22 +18,15 @@ export class PromiseBuffer { /** * Add a promise to the queue. * - * @param taskProducer A function producing any PromiseLike; In previous versions this used to be `@param task: PromiseLike`, however, Promises were instantly created on the call-site, making them fall through the buffer limit. + * @param taskProducer A function producing any PromiseLike; In previous versions this used to be `task: PromiseLike`, + * however, Promises were instantly created on the call-site, making them fall through the buffer limit. * @returns The original promise. */ - public add(taskProducer: PromiseLike | TaskProducer): PromiseLike { - // NOTE: This is necessary to preserve backwards compatibility - // It should accept _only_ `TaskProducer` 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 = isThenable(taskProducer) - ? () => taskProducer as PromiseLike - : (taskProducer as TaskProducer); - + public add(taskProducer: () => PromiseLike): PromiseLike { 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); } diff --git a/packages/utils/test/promisebuffer.test.ts b/packages/utils/test/promisebuffer.test.ts index 0a6a067bbc16..47b3cc8a5444 100644 --- a/packages/utils/test/promisebuffer.test.ts +++ b/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(); - const p = jest.fn( - () => new SyncPromise(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(1); - let t1; - const p1 = jest.fn(() => { - t1 = new SyncPromise(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(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(); - const p = new SyncPromise(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(); - const p = new SyncPromise(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(); - for (let i = 0; i < 5; i++) { - const p = new SyncPromise(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(); - for (let i = 0; i < 5; i++) { - const p = new SyncPromise(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(); - 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(); + const producer = () => new SyncPromise(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(); - const p = new SyncPromise((_, reject) => setTimeout(reject, 1)); - jest.runAllTimers(); - return q.add(p).then(null, () => { - expect(true).toBe(true); - }); + const buffer = new PromiseBuffer(); + const producer = () => new SyncPromise((_, reject) => setTimeout(() => reject(new Error('whoops')))); + const task = buffer.add(producer); + try { + await task; + } catch (e) { + expect(e).toEqual(new Error('whoops')); + } }); });