From 8d36cce0804d6bd4c044cae80cf21d5595445813 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Wed, 23 Jun 2021 12:47:17 +0200 Subject: [PATCH] fix: Correctly limit Buffer requests --- packages/browser/src/transports/fetch.ts | 35 +++---- packages/browser/src/transports/xhr.ts | 37 ++++---- .../test/unit/mocks/simpletransport.ts | 2 +- packages/core/test/mocks/transport.ts | 11 ++- packages/node/src/transports/base/index.ts | 91 ++++++++++--------- packages/utils/src/promisebuffer.ts | 16 +++- packages/utils/test/promisebuffer.test.ts | 24 +++-- 7 files changed, 121 insertions(+), 95 deletions(-) diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 11ebb38adccb..5c3c6ebd2b8c 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -133,23 +133,24 @@ export class FetchTransport extends BaseTransport { } return this._buffer.add( - new SyncPromise((resolve, reject) => { - void this._fetch(sentryRequest.url, options) - .then(response => { - const headers = { - 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), - 'retry-after': response.headers.get('Retry-After'), - }; - this._handleResponse({ - requestType: sentryRequest.type, - response, - headers, - resolve, - reject, - }); - }) - .catch(reject); - }), + () => + new SyncPromise((resolve, reject) => { + void this._fetch(sentryRequest.url, options) + .then(response => { + const headers = { + 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), + 'retry-after': response.headers.get('Retry-After'), + }; + this._handleResponse({ + requestType: sentryRequest.type, + response, + headers, + resolve, + reject, + }); + }) + .catch(reject); + }), ); } } diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index f33d6f28857f..e7f4c735baa1 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -37,27 +37,28 @@ export class XHRTransport extends BaseTransport { } return this._buffer.add( - new SyncPromise((resolve, reject) => { - const request = new XMLHttpRequest(); + () => + new SyncPromise((resolve, reject) => { + const request = new XMLHttpRequest(); - request.onreadystatechange = (): void => { - if (request.readyState === 4) { - const headers = { - 'x-sentry-rate-limits': request.getResponseHeader('X-Sentry-Rate-Limits'), - 'retry-after': request.getResponseHeader('Retry-After'), - }; - this._handleResponse({ requestType: sentryRequest.type, response: request, headers, resolve, reject }); - } - }; + request.onreadystatechange = (): void => { + if (request.readyState === 4) { + const headers = { + 'x-sentry-rate-limits': request.getResponseHeader('X-Sentry-Rate-Limits'), + 'retry-after': request.getResponseHeader('Retry-After'), + }; + this._handleResponse({ requestType: sentryRequest.type, response: request, headers, resolve, reject }); + } + }; - request.open('POST', sentryRequest.url); - for (const header in this.options.headers) { - if (this.options.headers.hasOwnProperty(header)) { - request.setRequestHeader(header, this.options.headers[header]); + request.open('POST', sentryRequest.url); + for (const header in this.options.headers) { + if (this.options.headers.hasOwnProperty(header)) { + request.setRequestHeader(header, this.options.headers[header]); + } } - } - request.send(sentryRequest.body); - }), + request.send(sentryRequest.body); + }), ); } } diff --git a/packages/browser/test/unit/mocks/simpletransport.ts b/packages/browser/test/unit/mocks/simpletransport.ts index 2f3cf591ba4e..01e0d6b6b12c 100644 --- a/packages/browser/test/unit/mocks/simpletransport.ts +++ b/packages/browser/test/unit/mocks/simpletransport.ts @@ -5,7 +5,7 @@ import { BaseTransport } from '../../../src/transports'; export class SimpleTransport extends BaseTransport { public sendEvent(_: Event): PromiseLike { - return this._buffer.add( + return this._buffer.add(() => SyncPromise.resolve({ status: Status.fromHttpCode(200), }), diff --git a/packages/core/test/mocks/transport.ts b/packages/core/test/mocks/transport.ts index e437427fd357..7b23c895d9da 100644 --- a/packages/core/test/mocks/transport.ts +++ b/packages/core/test/mocks/transport.ts @@ -16,11 +16,12 @@ export class FakeTransport implements Transport { public sendEvent(_event: Event): PromiseLike { this.sendCalled += 1; return this._buffer.add( - new SyncPromise(async res => { - await sleep(this.delay); - this.sentCount += 1; - res({ status: Status.Success }); - }), + () => + new SyncPromise(async res => { + await sleep(this.delay); + this.sentCount += 1; + res({ status: Status.Success }); + }), ); } diff --git a/packages/node/src/transports/base/index.ts b/packages/node/src/transports/base/index.ts index 1fbd6604554a..5868d47e9065 100644 --- a/packages/node/src/transports/base/index.ts +++ b/packages/node/src/transports/base/index.ts @@ -203,59 +203,62 @@ export abstract class BaseTransport implements Transport { return Promise.reject(new SentryError('Not adding Promise due to buffer limit reached.')); } return this._buffer.add( - new Promise((resolve, reject) => { - if (!this.module) { - throw new SentryError('No module available'); - } - const options = this._getRequestOptions(this.urlParser(sentryRequest.url)); - const req = this.module.request(options, res => { - const statusCode = res.statusCode || 500; - const status = Status.fromHttpCode(statusCode); + () => + new Promise((resolve, reject) => { + if (!this.module) { + throw new SentryError('No module available'); + } + const options = this._getRequestOptions(this.urlParser(sentryRequest.url)); + const req = this.module.request(options, res => { + const statusCode = res.statusCode || 500; + const status = Status.fromHttpCode(statusCode); - res.setEncoding('utf8'); + res.setEncoding('utf8'); - /** - * "Key-value pairs of header names and values. Header names are lower-cased." - * https://nodejs.org/api/http.html#http_message_headers - */ - let retryAfterHeader = res.headers ? res.headers['retry-after'] : ''; - retryAfterHeader = (Array.isArray(retryAfterHeader) ? retryAfterHeader[0] : retryAfterHeader) as string; + /** + * "Key-value pairs of header names and values. Header names are lower-cased." + * https://nodejs.org/api/http.html#http_message_headers + */ + let retryAfterHeader = res.headers ? res.headers['retry-after'] : ''; + retryAfterHeader = (Array.isArray(retryAfterHeader) ? retryAfterHeader[0] : retryAfterHeader) as string; - let rlHeader = res.headers ? res.headers['x-sentry-rate-limits'] : ''; - rlHeader = (Array.isArray(rlHeader) ? rlHeader[0] : rlHeader) as string; + let rlHeader = res.headers ? res.headers['x-sentry-rate-limits'] : ''; + rlHeader = (Array.isArray(rlHeader) ? rlHeader[0] : rlHeader) as string; - const headers = { - 'x-sentry-rate-limits': rlHeader, - 'retry-after': retryAfterHeader, - }; + const headers = { + 'x-sentry-rate-limits': rlHeader, + 'retry-after': retryAfterHeader, + }; - const limited = this._handleRateLimit(headers); - if (limited) - logger.warn( - `Too many ${sentryRequest.type} requests, backing off until: ${this._disabledUntil(sentryRequest.type)}`, - ); + const limited = this._handleRateLimit(headers); + if (limited) + logger.warn( + `Too many ${sentryRequest.type} requests, backing off until: ${this._disabledUntil( + sentryRequest.type, + )}`, + ); - if (status === Status.Success) { - resolve({ status }); - } else { - let rejectionMessage = `HTTP Error (${statusCode})`; - if (res.headers && res.headers['x-sentry-error']) { - rejectionMessage += `: ${res.headers['x-sentry-error']}`; + if (status === Status.Success) { + resolve({ status }); + } else { + let rejectionMessage = `HTTP Error (${statusCode})`; + if (res.headers && res.headers['x-sentry-error']) { + rejectionMessage += `: ${res.headers['x-sentry-error']}`; + } + reject(new SentryError(rejectionMessage)); } - reject(new SentryError(rejectionMessage)); - } - // Force the socket to drain - res.on('data', () => { - // Drain + // Force the socket to drain + res.on('data', () => { + // Drain + }); + res.on('end', () => { + // Drain + }); }); - res.on('end', () => { - // Drain - }); - }); - req.on('error', reject); - req.end(sentryRequest.body); - }), + req.on('error', reject); + req.end(sentryRequest.body); + }), ); } } diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index ff823048304b..bdb3f412f7ee 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -1,6 +1,9 @@ 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 */ @@ -18,13 +21,22 @@ export class PromiseBuffer { /** * Add a promise to the queue. * - * @param task Can be any PromiseLike + * @param taskProducer A function producing any PromiseLike * @returns The original promise. */ - public add(task: PromiseLike): PromiseLike { + 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); + if (!this.isReady()) { return SyncPromise.reject(new SentryError('Not adding Promise due to buffer limit reached.')); } + const task = normalizedTaskProducer(); 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 bf4288000f38..0a6a067bbc16 100644 --- a/packages/utils/test/promisebuffer.test.ts +++ b/packages/utils/test/promisebuffer.test.ts @@ -10,20 +10,28 @@ describe('PromiseBuffer', () => { describe('add()', () => { test('no limit', () => { const q = new PromiseBuffer(); - const p = new SyncPromise(resolve => setTimeout(resolve, 1)); + const p = jest.fn( + () => new SyncPromise(resolve => setTimeout(resolve, 1)), + ); q.add(p); expect(q.length()).toBe(1); }); + test('with limit', () => { const q = new PromiseBuffer(1); - const p = new SyncPromise(resolve => setTimeout(resolve, 1)); - expect(q.add(p)).toEqual(p); - expect( - q.add( - new SyncPromise(resolve => setTimeout(resolve, 1)), - ), - ).rejects.toThrowError(); + let t1; + const p1 = jest.fn(() => { + t1 = new SyncPromise(resolve => setTimeout(resolve, 1)); + return t1; + }); + 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(); }); });