diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 1597bddac91c..a9a241c35941 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -76,8 +76,8 @@ export abstract class BaseClient implement /** Array of used integrations. */ protected _integrations: IntegrationIndex = {}; - /** Number of call being processed */ - protected _processing: number = 0; + /** Number of calls being processed */ + protected _numProcessing: number = 0; /** * Initializes this client instance. @@ -185,11 +185,11 @@ export abstract class BaseClient implement * @inheritDoc */ public flush(timeout?: number): PromiseLike { - return this._isClientProcessing(timeout).then(ready => { + return this._isClientDoneProcessing(timeout).then(clientFinished => { return this._getBackend() .getTransport() .close(timeout) - .then(transportFlushed => ready && transportFlushed); + .then(transportFlushed => clientFinished && transportFlushed); }); } @@ -262,14 +262,23 @@ export abstract class BaseClient implement this._getBackend().sendSession(session); } - /** Waits for the client to be done with processing. */ - protected _isClientProcessing(timeout?: number): PromiseLike { + /** + * Determine if the client is finished processing. Returns a promise because it will wait `timeout` ms before saying + * "no" (resolving to `false`) in order to give the client a chance to potentially finish first. + * + * @param timeout The time, in ms, after which to resolve to `false` if the client is still busy. Passing `0` (or not + * passing anything) will make the promise wait as long as it takes for processing to finish before resolving to + * `true`. + * @returns A promise which will resolve to `true` if processing is already done or finishes before the timeout, and + * `false` otherwise + */ + protected _isClientDoneProcessing(timeout?: number): PromiseLike { return new SyncPromise(resolve => { let ticked: number = 0; const tick: number = 1; const interval = setInterval(() => { - if (this._processing == 0) { + if (this._numProcessing == 0) { clearInterval(interval); resolve(true); } else { @@ -554,14 +563,14 @@ export abstract class BaseClient implement * Occupies the client with processing and event */ protected _process(promise: PromiseLike): void { - this._processing += 1; + this._numProcessing += 1; void promise.then( value => { - this._processing -= 1; + this._numProcessing -= 1; return value; }, reason => { - this._processing -= 1; + this._numProcessing -= 1; return reason; }, ); diff --git a/packages/utils/src/promisebuffer.ts b/packages/utils/src/promisebuffer.ts index b27d039cf0b2..ff81d8dcddd8 100644 --- a/packages/utils/src/promisebuffer.ts +++ b/packages/utils/src/promisebuffer.ts @@ -16,33 +16,40 @@ export class PromiseBuffer { } /** - * Add a promise to the queue. + * Add a promise (representing an in-flight action) to the queue, and set it to remove itself on fulfillment. * - * @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. + * @param taskProducer A function producing any PromiseLike; In previous versions this used to be `task: + * PromiseLike`, but under that model, Promises were instantly created on the call-site and their executor + * functions therefore ran immediately. Thus, even if the buffer was full, the action still happened. By + * requiring the promise to be wrapped in a function, we can defer promise creation until after the buffer + * limit check. * @returns The original promise. */ public add(taskProducer: () => PromiseLike): PromiseLike { if (!this.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); } void task .then(() => this.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 this catch here otherwise we have an unhandledPromiseRejection - // because it's a new Promise chain. + // We have to add another catch here because `this.remove()` starts a new promise chain. }), ); return task; } /** - * Remove a promise to the queue. + * Remove a promise from the queue. * * @param task Can be any PromiseLike * @returns Removed promise. @@ -60,18 +67,24 @@ export class PromiseBuffer { } /** - * This will drain the whole queue, returns true if queue is empty or drained. - * If timeout is provided and the queue takes longer to drain, the promise still resolves but with false. + * Wait for all promises in the queue to resolve or for timeout to expire, whichever comes first. * - * @param timeout Number in ms to wait until it resolves with false. + * @param timeout The time, in ms, after which to resolve to `false` if the queue is still non-empty. Passing `0` (or + * not passing anything) will make the promise wait as long as it takes for the queue to drain before resolving to + * `true`. + * @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 { return new SyncPromise(resolve => { + // wait for `timeout` ms and then resolve to `false` (if not cancelled first) const capturedSetTimeout = setTimeout(() => { if (timeout && timeout > 0) { resolve(false); } }, timeout); + + // if all promises resolve in time, cancel the timer and resolve to `true` void SyncPromise.all(this._buffer) .then(() => { clearTimeout(capturedSetTimeout);