docs(core): Add flush-related docstrings and comments (#3789)

Annotations added in the process of making sense of the code while debugging something else.

In addition to docstrings and comments, the only changes are:

- s/`_isClientProcessing`/`_isClientDoneProcessing`: This resolves to `true` when processing is done, and `false` if processing is still happening, which is the opposite of what the original name implied.

- s/`_processing`/`_numProcessing`:  Since it represents a number, might as well make that obvious in the name, lest the casual reader think it might be a boolean or a collection of things currently being processed.

- s/`ready`/`clientFinished` in `flush`: This provides a nice parallel to `transportFlushed`. Also, the client may well be "ready," but since this is mostly called as part of a shutdown procedure, the fact that it's ready matters less than the fact that it's finished with the work it was doing.

Author: lobsterkatie

packages/core/src/baseclient.ts

@@ -76,8 +76,8 @@ export abstract class BaseClient<B extends Backend, O extends Options> 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<B extends Backend, O extends Options> implement
    * @inheritDoc
    */
   public flush(timeout?: number): PromiseLike<boolean> {
-    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<B extends Backend, O extends Options> implement
     this._getBackend().sendSession(session);
   }
 
-  /** Waits for the client to be done with processing. */
-  protected _isClientProcessing(timeout?: number): PromiseLike<boolean> {
+  /**
+   * 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<boolean> {
     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<B extends Backend, O extends Options> implement
    * Occupies the client with processing and event
    */
   protected _process<T>(promise: PromiseLike<T>): 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;
       },
     );

packages/utils/src/promisebuffer.ts

@@ -16,33 +16,40 @@ export class PromiseBuffer<T> {
   }
 
   /**
-   * 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<T>; In previous versions this used to be `task: PromiseLike<T>`,
-   *        however, Promises were instantly created on the call-site, making them fall through the buffer limit.
+   * @param taskProducer A function producing any PromiseLike<T>; In previous versions this used to be `task:
+   *        PromiseLike<T>`, 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<T>): PromiseLike<T> {
     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<T>
    * @returns Removed promise.
@@ -60,18 +67,24 @@ export class PromiseBuffer<T> {
   }
 
   /**
-   * 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<boolean> {
     return new SyncPromise<boolean>(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);