fix: Wait for processing to finish before closing transport (#2005)

* fix: Wait for processing to finish before closing transport

* fix: Remove finally

* fix: Remove node flush tests

* fix: Reenable integration tests

* meta: Changelog

* fix: Integrations

* fix: Make setInterval number

* fix: Don't use window

Author: HazAT

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - [utils] ref: Move `htmlTreeAsString` to `@sentry/browser`
 - [utils] ref: Remove `Window` typehint `getGlobalObject`
+- [core] fix: Make sure that flush/close works as advertised
 
 ## 5.0.6
 

packages/browser/test/index.test.ts

@@ -143,7 +143,7 @@ describe('SentryBrowser', () => {
       captureMessage('event222');
       captureMessage('event222');
 
-      await flush(2000);
+      await flush(10);
 
       expect(localBeforeSend.calledTwice).to.be.true;
     });

packages/browser/test/integration/test.js

@@ -64,7 +64,7 @@ function _alt(title, condition) {
   return condition ? '⚠️   Skipped: ' + title : title;
 }
 
-var frames = ['frame'];
+var frames = ['frame', 'loader', 'loader-lazy-no'];
 
 for (var idx in frames) {
   (function() {
@@ -911,7 +911,7 @@ for (var idx in frames) {
           );
         });
 
-        it.only("should capture built-in's handlers fn name in mechanism data", function(done) {
+        it("should capture built-in's handlers fn name in mechanism data", function(done) {
           var iframe = this.iframe;
 
           iframeExecute(

packages/core/src/baseclient.ts

@@ -58,6 +58,9 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
   /** Is the client still processing a call? */
   protected _processing: boolean = false;
 
+  /** Processing interval */
+  protected _processingInterval?: number;
+
   /**
    * Initializes this client instance.
    *
@@ -163,21 +166,22 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
    * @inheritDoc
    */
   public async flush(timeout?: number): Promise<boolean> {
-    return (await Promise.all([
-      this._getBackend()
-        .getTransport()
-        .close(timeout),
-      this._isClientProcessing(),
-    ])).reduce((prev, current) => prev && current);
+    const clientReady = await this._isClientProcessing(timeout);
+    if (this._processingInterval) {
+      clearInterval(this._processingInterval);
+    }
+    const transportFlushed = await this._getBackend()
+      .getTransport()
+      .close(timeout);
+    return clientReady && transportFlushed;
   }
 
   /**
    * @inheritDoc
    */
   public async close(timeout?: number): Promise<boolean> {
-    return this.flush(timeout).finally(() => {
-      this.getOptions().enabled = false;
-    });
+    this.getOptions().enabled = false;
+    return this.flush(timeout);
   }
 
   /**
@@ -200,20 +204,23 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
   }
 
   /** Waits for the client to be done with processing. */
-  protected async _isClientProcessing(counter: number = 0): Promise<boolean> {
+  protected async _isClientProcessing(timeout?: number): Promise<boolean> {
     return new Promise<boolean>(resolve => {
-      if (this._processing) {
-        // Safeguard in case of endless recursion
-        if (counter >= 10) {
-          resolve(false);
+      let ticked: number = 0;
+      const tick: number = 1;
+      if (this._processingInterval) {
+        clearInterval(this._processingInterval);
+      }
+      this._processingInterval = (setInterval(() => {
+        if (!this._processing) {
+          resolve(true);
         } else {
-          setTimeout(async () => {
-            resolve(await this._isClientProcessing(counter + 1));
-          }, 10);
+          ticked += tick;
+          if (timeout && ticked >= timeout) {
+            resolve(false);
+          }
         }
-      } else {
-        resolve(true);
-      }
+      }, tick) as unknown) as number;
     });
   }
 

packages/core/test/lib/base.test.ts

@@ -5,6 +5,7 @@ import { SentryError } from '@sentry/utils';
 import { TestBackend } from '../mocks/backend';
 import { TestClient } from '../mocks/client';
 import { TestIntegration } from '../mocks/integration';
+import { FakeTransport } from '../mocks/transport';
 
 const PUBLIC_DSN = 'https://username@domain/path';
 
@@ -427,4 +428,47 @@ describe('BaseClient', () => {
       expect(client.getIntegration(TestIntegration)).toBeTruthy();
     });
   });
+
+  describe('flush/close', () => {
+    test('flush', async () => {
+      jest.useRealTimers();
+      expect.assertions(5);
+      const client = new TestClient({
+        dsn: PUBLIC_DSN,
+        enableSend: true,
+        transport: FakeTransport,
+      });
+
+      const delay = 1;
+      const transportInstance = (client as any)._getBackend().getTransport() as FakeTransport;
+      transportInstance.delay = delay;
+
+      client.captureMessage('test');
+      expect(transportInstance).toBeInstanceOf(FakeTransport);
+      expect(transportInstance.sendCalled).toEqual(1);
+      expect(transportInstance.sentCount).toEqual(0);
+      await client.flush(delay);
+      expect(transportInstance.sentCount).toEqual(1);
+      expect(transportInstance.sendCalled).toEqual(1);
+    });
+
+    test('close', async () => {
+      jest.useRealTimers();
+      expect.assertions(2);
+      const client = new TestClient({
+        dsn: PUBLIC_DSN,
+        enableSend: true,
+        transport: FakeTransport,
+      });
+
+      const delay = 1;
+      const transportInstance = (client as any)._getBackend().getTransport() as FakeTransport;
+      transportInstance.delay = delay;
+
+      expect(client.captureMessage('test')).toBeTruthy();
+      await client.close(delay);
+      // Sends after close shouldn't work anymore
+      expect(client.captureMessage('test')).toBeFalsy();
+    });
+  });
 });

packages/core/test/mocks/backend.ts

@@ -1,11 +1,12 @@
-import { Event, Options } from '@sentry/types';
+import { Event, Options, Transport } from '@sentry/types';
 import { SyncPromise } from '@sentry/utils';
 
 import { BaseBackend } from '../../src/basebackend';
 
 export interface TestOptions extends Options {
   test?: boolean;
   mockInstallFailure?: boolean;
+  enableSend?: boolean;
 }
 
 export class TestBackend extends BaseBackend<TestOptions> {
@@ -19,6 +20,23 @@ export class TestBackend extends BaseBackend<TestOptions> {
     TestBackend.instance = this;
   }
 
+  protected _setupTransport(): Transport {
+    if (!this._options.dsn) {
+      // We return the noop transport here in case there is no Dsn.
+      return super._setupTransport();
+    }
+
+    const transportOptions = this._options.transportOptions
+      ? this._options.transportOptions
+      : { dsn: this._options.dsn };
+
+    if (this._options.transport) {
+      return new this._options.transport(transportOptions);
+    }
+
+    return super._setupTransport();
+  }
+
   public eventFromException(exception: any): SyncPromise<Event> {
     return SyncPromise.resolve({
       exception: {
@@ -38,6 +56,10 @@ export class TestBackend extends BaseBackend<TestOptions> {
 
   public sendEvent(event: Event): void {
     this.event = event;
+    if (this._options.enableSend) {
+      super.sendEvent(event);
+      return;
+    }
     // tslint:disable-next-line
     TestBackend.sendEventCalled && TestBackend.sendEventCalled(event);
   }

packages/core/test/mocks/transport.ts

@@ -0,0 +1,30 @@
+import { Event, Response, Status, Transport } from '@sentry/types';
+import { PromiseBuffer } from '@sentry/utils';
+
+async function sleep(delay: number): Promise<void> {
+  return new Promise(resolve => setTimeout(resolve, delay));
+}
+
+export class FakeTransport implements Transport {
+  /** A simple buffer holding all requests. */
+  protected readonly _buffer: PromiseBuffer<Response> = new PromiseBuffer(9999);
+
+  public sendCalled: number = 0;
+  public sentCount: number = 0;
+  public delay: number = 2000;
+
+  public async sendEvent(_event: Event): Promise<Response> {
+    this.sendCalled += 1;
+    return this._buffer.add(
+      new Promise(async res => {
+        await sleep(this.delay);
+        this.sentCount += 1;
+        res({ status: Status.Success });
+      }),
+    );
+  }
+
+  public async close(timeout?: number): Promise<boolean> {
+    return this._buffer.drain(timeout);
+  }
+}

packages/integrations/src/captureconsole.ts

@@ -1,7 +1,5 @@
 import { EventProcessor, Hub, Integration, Severity } from '@sentry/types';
-import { getGlobalObject } from '@sentry/utils/misc';
-import { fill, normalize } from '@sentry/utils/object';
-import { safeJoin } from '@sentry/utils/string';
+import { fill, getGlobalObject, normalize, safeJoin } from '@sentry/utils';
 
 const global = getGlobalObject<Window | NodeJS.Global>();
 
@@ -39,49 +37,46 @@ export class CaptureConsole implements Integration {
       return;
     }
 
-    this._levels.forEach(function(level: string): void {
+    this._levels.forEach((level: string) => {
       if (!(level in global.console)) {
         return;
       }
 
-      fill(global.console, level, function(originalConsoleLevel: () => any): any {
-        // tslint:disable-next-line:only-arrow-functions
-        return function(...args: any[]): any {
-          const hub = getCurrentHub();
+      fill(global.console, level, (originalConsoleLevel: () => any) => (...args: any[]) => {
+        const hub = getCurrentHub();
 
-          if (hub.getIntegration(CaptureConsole)) {
-            hub.withScope(scope => {
-              scope.setLevel(Severity.fromString(level));
-              scope.setExtra('arguments', normalize(args, 3));
-              scope.addEventProcessor(event => {
-                event.logger = 'console';
-                if (event.sdk) {
-                  event.sdk = {
-                    ...event.sdk,
-                    integrations: [...(event.sdk.integrations || []), 'console'],
-                  };
-                }
-                return event;
-              });
+        if (hub.getIntegration(CaptureConsole)) {
+          hub.withScope(scope => {
+            scope.setLevel(Severity.fromString(level));
+            scope.setExtra('arguments', normalize(args, 3));
+            scope.addEventProcessor(event => {
+              event.logger = 'console';
+              if (event.sdk) {
+                event.sdk = {
+                  ...event.sdk,
+                  integrations: [...(event.sdk.integrations || []), 'console'],
+                };
+              }
+              return event;
+            });
 
-              let message = safeJoin(args, ' ');
-              if (level === 'assert') {
-                if (args[0] === false) {
-                  message = `Assertion failed: ${safeJoin(args.slice(1), ' ') || 'console.assert'}`;
-                  scope.setExtra('arguments', normalize(args.slice(1), 3));
-                  hub.captureMessage(message);
-                }
-              } else {
+            let message = safeJoin(args, ' ');
+            if (level === 'assert') {
+              if (args[0] === false) {
+                message = `Assertion failed: ${safeJoin(args.slice(1), ' ') || 'console.assert'}`;
+                scope.setExtra('arguments', normalize(args.slice(1), 3));
                 hub.captureMessage(message);
               }
-            });
-          }
+            } else {
+              hub.captureMessage(message);
+            }
+          });
+        }
 
-          // this fails for some browsers. :(
-          if (originalConsoleLevel) {
-            Function.prototype.apply.call(originalConsoleLevel, global.console, args);
-          }
-        };
+        // this fails for some browsers. :(
+        if (originalConsoleLevel) {
+          Function.prototype.apply.call(originalConsoleLevel, global.console, args);
+        }
       });
     });
   }

packages/integrations/src/debug.ts

@@ -1,5 +1,5 @@
 import { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types';
-import { consoleSandbox } from '@sentry/utils/misc';
+import { consoleSandbox } from '@sentry/utils';
 
 /** JSDoc */
 interface DebugOptions {

packages/node/test/index.test.ts

@@ -33,53 +33,6 @@ describe('SentryNode', () => {
     getCurrentHub().popScope();
   });
 
-  test('flush() with to short timeout', done => {
-    expect.assertions(1);
-    jest.useFakeTimers();
-    const client = new NodeClient({
-      dsn,
-      transport: SetTimeoutTransport,
-    });
-    getCurrentHub().bindClient(client);
-    captureMessage('test');
-    captureMessage('test');
-    captureMessage('test');
-    client
-      .flush(50)
-      .then(result => {
-        expect(result).toBeFalsy();
-        done();
-      })
-      .catch(() => {
-        // test
-      });
-    jest.runAllTimers();
-  });
-
-  test('flush() with timeout', done => {
-    expect.assertions(1);
-    jest.useFakeTimers();
-    const client = new NodeClient({
-      dsn,
-      transport: SetTimeoutTransport,
-    });
-    getCurrentHub().bindClient(client);
-    captureMessage('test');
-    captureMessage('test');
-    captureMessage('test');
-    jest.runAllTimers();
-    client
-      .flush(50)
-      .then(result => {
-        expect(result).toBeFalsy();
-        done();
-      })
-      .catch(() => {
-        // test
-      });
-    jest.runAllTimers();
-  });
-
   describe('getContext() / setContext()', () => {
     test('store/load extra', async () => {
       configureScope((scope: Scope) => {