Skip to content

Commit 29b6914

Browse files
committed
ref(utils): refactor promisebuffer
1 parent 1fe4a0f commit 29b6914

File tree

5 files changed

+74
-50
lines changed

5 files changed

+74
-50
lines changed

packages/browser/src/transports/base.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
eventStatusFromHttpCode,
1818
getGlobalObject,
1919
logger,
20+
makePromiseBuffer,
2021
parseRetryAfterHeader,
2122
PromiseBuffer,
2223
SentryError,
@@ -42,7 +43,7 @@ export abstract class BaseTransport implements Transport {
4243
protected readonly _api: APIDetails;
4344

4445
/** A simple buffer holding all requests. */
45-
protected readonly _buffer: PromiseBuffer<SentryResponse> = new PromiseBuffer(30);
46+
protected readonly _buffer: PromiseBuffer<SentryResponse> = makePromiseBuffer(30);
4647

4748
/** Locks transport after receiving rate limits in a response */
4849
protected readonly _rateLimits: Record<string, Date> = {};

packages/core/test/mocks/transport.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Event, Response, Transport } from '@sentry/types';
2-
import { PromiseBuffer, SyncPromise } from '@sentry/utils';
2+
import { PromiseBuffer, makePromiseBuffer, SyncPromise } from '@sentry/utils';
33

44
async function sleep(delay: number): Promise<void> {
55
return new SyncPromise(resolve => setTimeout(resolve, delay));
@@ -11,7 +11,7 @@ export class FakeTransport implements Transport {
1111
public delay: number = 2000;
1212

1313
/** A simple buffer holding all requests. */
14-
protected readonly _buffer: PromiseBuffer<Response> = new PromiseBuffer(9999);
14+
protected readonly _buffer: PromiseBuffer<Response> = makePromiseBuffer(9999);
1515

1616
public sendEvent(_event: Event): PromiseLike<Response> {
1717
this.sendCalled += 1;

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,14 @@ import {
1010
Transport,
1111
TransportOptions,
1212
} from '@sentry/types';
13-
import { eventStatusFromHttpCode, logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils';
13+
import {
14+
eventStatusFromHttpCode,
15+
logger,
16+
parseRetryAfterHeader,
17+
PromiseBuffer,
18+
makePromiseBuffer,
19+
SentryError,
20+
} from '@sentry/utils';
1421
import * as fs from 'fs';
1522
import * as http from 'http';
1623
import * as https from 'https';
@@ -43,7 +50,7 @@ export abstract class BaseTransport implements Transport {
4350
protected _api: APIDetails;
4451

4552
/** A simple buffer holding all requests. */
46-
protected readonly _buffer: PromiseBuffer<Response> = new PromiseBuffer(30);
53+
protected readonly _buffer: PromiseBuffer<Response> = makePromiseBuffer(30);
4754

4855
/** Locks transport after receiving rate limits in a response */
4956
protected readonly _rateLimits: Record<string, Date> = {};

packages/utils/src/promisebuffer.ts

Lines changed: 51 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,41 @@ function allPromises<U = unknown>(collection: Array<U | PromiseLike<U>>): Promis
2222
});
2323
}
2424

25-
/** A simple queue that holds promises. */
26-
export class PromiseBuffer<T> {
27-
/** Internal set of queued Promises */
28-
private readonly _buffer: Array<PromiseLike<T>> = [];
25+
export interface PromiseBuffer<T> {
26+
_buffer: Array<PromiseLike<T>>;
27+
isReady(): boolean;
28+
add(taskProducer: () => PromiseLike<T>): PromiseLike<T>;
29+
remove(task: PromiseLike<T>): PromiseLike<T>;
30+
length(): number;
31+
drain(timeout?: number): PromiseLike<boolean>;
32+
}
33+
34+
/**
35+
* Creates an new PromiseBuffer object with the specified limit
36+
* @param limit max number of promises that can be stored in the buffer
37+
*/
38+
export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
39+
const buffer: Array<PromiseLike<T>> = [];
40+
41+
/**
42+
* This function returns the number of unresolved promises in the queue.
43+
*/
44+
function length(): number {
45+
return buffer.length;
46+
}
2947

30-
public constructor(protected _limit?: number) {}
48+
function isReady(): boolean {
49+
return limit === undefined || length() < limit;
50+
}
3151

3252
/**
33-
* Says if the buffer is ready to take more requests
53+
* Remove a promise from the queue.
54+
*
55+
* @param task Can be any PromiseLike<T>
56+
* @returns Removed promise.
3457
*/
35-
public isReady(): boolean {
36-
return this._limit === undefined || this.length() < this._limit;
58+
function remove(task: PromiseLike<T>): PromiseLike<T> {
59+
return buffer.splice(buffer.indexOf(task), 1)[0];
3760
}
3861

3962
/**
@@ -46,47 +69,29 @@ export class PromiseBuffer<T> {
4669
* limit check.
4770
* @returns The original promise.
4871
*/
49-
public add(taskProducer: () => PromiseLike<T>): PromiseLike<T> {
50-
if (!this.isReady()) {
72+
function add(taskProducer: () => PromiseLike<T>): PromiseLike<T> {
73+
if (!isReady()) {
5174
return SyncPromise.reject(new SentryError('Not adding Promise due to buffer limit reached.'));
5275
}
5376

5477
// start the task and add its promise to the queue
5578
const task = taskProducer();
56-
if (this._buffer.indexOf(task) === -1) {
57-
this._buffer.push(task);
79+
if (buffer.indexOf(task) === -1) {
80+
buffer.push(task);
5881
}
5982
void task
60-
.then(() => this.remove(task))
83+
.then(() => remove(task))
6184
// Use `then(null, rejectionHandler)` rather than `catch(rejectionHandler)` so that we can use `PromiseLike`
6285
// rather than `Promise`. `PromiseLike` doesn't have a `.catch` method, making its polyfill smaller. (ES5 didn't
6386
// have promises, so TS has to polyfill when down-compiling.)
6487
.then(null, () =>
65-
this.remove(task).then(null, () => {
66-
// We have to add another catch here because `this.remove()` starts a new promise chain.
88+
remove(task).then(null, () => {
89+
// We have to add another catch here because `remove()` starts a new promise chain.
6790
}),
6891
);
6992
return task;
7093
}
7194

72-
/**
73-
* Remove a promise from the queue.
74-
*
75-
* @param task Can be any PromiseLike<T>
76-
* @returns Removed promise.
77-
*/
78-
public remove(task: PromiseLike<T>): PromiseLike<T> {
79-
const removedTask = this._buffer.splice(this._buffer.indexOf(task), 1)[0];
80-
return removedTask;
81-
}
82-
83-
/**
84-
* This function returns the number of unresolved promises in the queue.
85-
*/
86-
public length(): number {
87-
return this._buffer.length;
88-
}
89-
9095
/**
9196
* Wait for all promises in the queue to resolve or for timeout to expire, whichever comes first.
9297
*
@@ -96,7 +101,7 @@ export class PromiseBuffer<T> {
96101
* @returns A promise which will resolve to `true` if the queue is already empty or drains before the timeout, and
97102
* `false` otherwise
98103
*/
99-
public drain(timeout?: number): PromiseLike<boolean> {
104+
function drain(timeout?: number): PromiseLike<boolean> {
100105
return new SyncPromise<boolean>(resolve => {
101106
// wait for `timeout` ms and then resolve to `false` (if not cancelled first)
102107
const capturedSetTimeout = setTimeout(() => {
@@ -106,10 +111,21 @@ export class PromiseBuffer<T> {
106111
}, timeout);
107112

108113
// if all promises resolve in time, cancel the timer and resolve to `true`
109-
void allPromises(this._buffer).then(() => {
114+
void allPromises(buffer).then(() => {
110115
clearTimeout(capturedSetTimeout);
111116
resolve(true);
112117
});
113118
});
114119
}
120+
121+
const promiseBuffer: PromiseBuffer<T> = {
122+
_buffer: buffer,
123+
length,
124+
isReady,
125+
add,
126+
remove,
127+
drain,
128+
};
129+
130+
return promiseBuffer;
115131
}

packages/utils/test/promisebuffer.test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
1-
import { PromiseBuffer } from '../src/promisebuffer';
1+
import { makePromiseBuffer } from '../src/promisebuffer';
22
import { SyncPromise } from '../src/syncpromise';
33

44
describe('PromiseBuffer', () => {
55
describe('add()', () => {
66
test('no limit', () => {
7-
const buffer = new PromiseBuffer();
7+
const buffer = makePromiseBuffer();
88
const p = jest.fn(() => new SyncPromise(resolve => setTimeout(resolve)));
99
void buffer.add(p);
1010
expect(buffer.length()).toEqual(1);
1111
});
1212

1313
test('with limit', () => {
14-
const buffer = new PromiseBuffer(1);
14+
const buffer = makePromiseBuffer(1);
1515
let task1;
1616
const producer1 = jest.fn(() => {
1717
task1 = new SyncPromise(resolve => setTimeout(resolve));
@@ -28,7 +28,7 @@ describe('PromiseBuffer', () => {
2828

2929
describe('drain()', () => {
3030
test('without timeout', async () => {
31-
const buffer = new PromiseBuffer();
31+
const buffer = makePromiseBuffer();
3232
for (let i = 0; i < 5; i++) {
3333
void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve)));
3434
}
@@ -39,7 +39,7 @@ describe('PromiseBuffer', () => {
3939
});
4040

4141
test('with timeout', async () => {
42-
const buffer = new PromiseBuffer();
42+
const buffer = makePromiseBuffer();
4343
for (let i = 0; i < 5; i++) {
4444
void buffer.add(() => new SyncPromise(resolve => setTimeout(resolve, 100)));
4545
}
@@ -49,7 +49,7 @@ describe('PromiseBuffer', () => {
4949
});
5050

5151
test('on empty buffer', async () => {
52-
const buffer = new PromiseBuffer();
52+
const buffer = makePromiseBuffer();
5353
expect(buffer.length()).toEqual(0);
5454
const result = await buffer.drain();
5555
expect(result).toEqual(true);
@@ -58,7 +58,7 @@ describe('PromiseBuffer', () => {
5858
});
5959

6060
test('resolved promises should not show up in buffer length', async () => {
61-
const buffer = new PromiseBuffer();
61+
const buffer = makePromiseBuffer();
6262
const producer = () => new SyncPromise(resolve => setTimeout(resolve));
6363
const task = buffer.add(producer);
6464
expect(buffer.length()).toEqual(1);
@@ -67,7 +67,7 @@ describe('PromiseBuffer', () => {
6767
});
6868

6969
test('rejected promises should not show up in buffer length', async () => {
70-
const buffer = new PromiseBuffer();
70+
const buffer = makePromiseBuffer();
7171
const producer = () => new SyncPromise((_, reject) => setTimeout(reject));
7272
const task = buffer.add(producer);
7373
expect(buffer.length()).toEqual(1);
@@ -80,7 +80,7 @@ describe('PromiseBuffer', () => {
8080
});
8181

8282
test('resolved task should give an access to the return value', async () => {
83-
const buffer = new PromiseBuffer<string>();
83+
const buffer = makePromiseBuffer<string>();
8484
const producer = () => new SyncPromise<string>(resolve => setTimeout(() => resolve('test')));
8585
const task = buffer.add(producer);
8686
const result = await task;
@@ -89,7 +89,7 @@ describe('PromiseBuffer', () => {
8989

9090
test('rejected task should give an access to the return value', async () => {
9191
expect.assertions(1);
92-
const buffer = new PromiseBuffer<string>();
92+
const buffer = makePromiseBuffer<string>();
9393
const producer = () => new SyncPromise<string>((_, reject) => setTimeout(() => reject(new Error('whoops'))));
9494
const task = buffer.add(producer);
9595
try {

0 commit comments

Comments
 (0)