fix: Rewrite transport and queue to prevent memory leaks

CHANGELOG.md

@@ -2,6 +2,15 @@
 
 ## Unreleased
 
+- [core] feat: Deprecate `captureEvent`, prefer `sendEvent` for transports. `sendEvent` now takes a string (body)
+  instead of `Event` object.
+- [core] feat: Use correct buffer for requests in transports
+- [node] feat: Add file cache for providing pre/post context in frames
+- [node] feat: New option `frameContextLines`, if set to `0` we do not provide source code pre/post context, default is
+  `7` lines pre/post
+- [core]: ref: Change way how transports are initialized
+- [core]: ref: Rename `RequestBuffer` to `PromiseBuffer`, also introduce limit
+
 ## 4.4.2
 
 - [node] Port memory-leak tests from raven-node

packages/browser/src/backend.ts

@@ -1,7 +1,6 @@
 import { BaseBackend, Options, SentryError } from '@sentry/core';
-import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status } from '@sentry/types';
+import { SentryEvent, SentryEventHint, Severity, Transport } from '@sentry/types';
 import { isDOMError, isDOMException, isError, isErrorEvent, isPlainObject } from '@sentry/utils/is';
-import { logger } from '@sentry/utils/logger';
 import { supportsBeacon, supportsFetch } from '@sentry/utils/supports';
 import { eventFromPlainObject, eventFromStacktrace, prepareFramesForEvent } from './parsers';
 import { computeStackTrace } from './tracekit';
@@ -46,6 +45,27 @@ export class BrowserBackend extends BaseBackend<BrowserOptions> {
     return true;
   }
 
+  /**
+   * @inheritdoc
+   */
+  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);
+    } else if (supportsBeacon()) {
+      return new BeaconTransport(transportOptions);
+    } else if (supportsFetch()) {
+      return new FetchTransport(transportOptions);
+    }
+    return new XHRTransport(transportOptions);
+  }
+
   /**
    * @inheritDoc
    */
@@ -126,33 +146,4 @@ export class BrowserBackend extends BaseBackend<BrowserOptions> {
 
     return event;
   }
-
-  /**
-   * @inheritDoc
-   */
-  public async sendEvent(event: SentryEvent): Promise<SentryResponse> {
-    if (!this.options.dsn) {
-      logger.warn(`Event has been skipped because no Dsn is configured.`);
-      // We do nothing in case there is no DSN
-      return { status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` };
-    }
-
-    if (!this.transport) {
-      const transportOptions = this.options.transportOptions
-        ? this.options.transportOptions
-        : { dsn: this.options.dsn };
-
-      if (this.options.transport) {
-        this.transport = new this.options.transport({ dsn: this.options.dsn });
-      } else if (supportsBeacon()) {
-        this.transport = new BeaconTransport(transportOptions);
-      } else if (supportsFetch()) {
-        this.transport = new FetchTransport(transportOptions);
-      } else {
-        this.transport = new XHRTransport(transportOptions);
-      }
-    }
-
-    return this.transport.captureEvent(event);
-  }
 }

packages/browser/src/transports/base.ts

@@ -1,5 +1,5 @@
-import { API, SentryError } from '@sentry/core';
-import { SentryEvent, SentryResponse, Transport, TransportOptions } from '@sentry/types';
+import { API, PromiseBuffer, SentryError } from '@sentry/core';
+import { SentryResponse, Transport, TransportOptions } from '@sentry/types';
 
 /** Base Transport class implementation */
 export abstract class BaseTransport implements Transport {
@@ -8,14 +8,24 @@ export abstract class BaseTransport implements Transport {
    */
   public url: string;
 
+  /** A simple buffer holding all requests. */
+  protected readonly buffer: PromiseBuffer<SentryResponse> = new PromiseBuffer(30);
+
   public constructor(public options: TransportOptions) {
     this.url = new API(this.options.dsn).getStoreEndpointWithUrlEncodedAuth();
   }
 
   /**
    * @inheritDoc
    */
-  public async captureEvent(_: SentryEvent): Promise<SentryResponse> {
-    throw new SentryError('Transport Class has to implement `captureEvent` method');
+  public async sendEvent(_: string): Promise<SentryResponse> {
+    throw new SentryError('Transport Class has to implement `sendEvent` method');
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public async close(timeout?: number): Promise<boolean> {
+    return this.buffer.drain(timeout);
   }
 }

packages/browser/src/transports/beacon.ts

@@ -1,6 +1,5 @@
-import { SentryEvent, SentryResponse, Status } from '@sentry/types';
+import { SentryResponse, Status } from '@sentry/types';
 import { getGlobalObject } from '@sentry/utils/misc';
-import { serialize } from '@sentry/utils/object';
 import { BaseTransport } from './base';
 
 const global = getGlobalObject() as Window;
@@ -10,13 +9,13 @@ export class BeaconTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
-    const data = serialize(event);
+  public async sendEvent(body: string): Promise<SentryResponse> {
+    const result = global.navigator.sendBeacon(this.url, body);
 
-    const result = global.navigator.sendBeacon(this.url, data);
-
-    return {
-      status: result ? Status.Success : Status.Failed,
-    };
+    return this.buffer.add(
+      Promise.resolve({
+        status: result ? Status.Success : Status.Failed,
+      }),
+    );
   }
 }

packages/browser/src/transports/fetch.ts

@@ -1,6 +1,5 @@
-import { SentryEvent, SentryResponse, Status } from '@sentry/types';
+import { SentryResponse, Status } from '@sentry/types';
 import { getGlobalObject } from '@sentry/utils/misc';
-import { serialize } from '@sentry/utils/object';
 import { supportsReferrerPolicy } from '@sentry/utils/supports';
 import { BaseTransport } from './base';
 
@@ -11,9 +10,9 @@ export class FetchTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
+  public async sendEvent(body: string): Promise<SentryResponse> {
     const defaultOptions: RequestInit = {
-      body: serialize(event),
+      body,
       method: 'POST',
       // Despite all stars in the sky saying that Edge supports old draft syntax, aka 'never', 'always', 'origin' and 'default
       // https://caniuse.com/#feat=referrer-policy
@@ -22,10 +21,10 @@ export class FetchTransport extends BaseTransport {
       referrerPolicy: (supportsReferrerPolicy() ? 'origin' : '') as ReferrerPolicy,
     };
 
-    const response = await global.fetch(this.url, defaultOptions);
-
-    return {
-      status: Status.fromHttpCode(response.status),
-    };
+    return this.buffer.add(
+      global.fetch(this.url, defaultOptions).then(response => ({
+        status: Status.fromHttpCode(response.status),
+      })),
+    );
   }
 }

packages/browser/src/transports/xhr.ts

@@ -1,32 +1,33 @@
-import { SentryEvent, SentryResponse, Status } from '@sentry/types';
-import { serialize } from '@sentry/utils/object';
+import { SentryResponse, Status } from '@sentry/types';
 import { BaseTransport } from './base';
 
 /** `XHR` based transport */
 export class XHRTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
-    return new Promise<SentryResponse>((resolve, reject) => {
-      const request = new XMLHttpRequest();
+  public async sendEvent(body: string): Promise<SentryResponse> {
+    return this.buffer.add(
+      new Promise<SentryResponse>((resolve, reject) => {
+        const request = new XMLHttpRequest();
 
-      request.onreadystatechange = () => {
-        if (request.readyState !== 4) {
-          return;
-        }
+        request.onreadystatechange = () => {
+          if (request.readyState !== 4) {
+            return;
+          }
 
-        if (request.status === 200) {
-          resolve({
-            status: Status.fromHttpCode(request.status),
-          });
-        }
+          if (request.status === 200) {
+            resolve({
+              status: Status.fromHttpCode(request.status),
+            });
+          }
 
-        reject(request);
-      };
+          reject(request);
+        };
 
-      request.open('POST', this.url);
-      request.send(serialize(event));
-    });
+        request.open('POST', this.url);
+        request.send(body);
+      }),
+    );
   }
 }

packages/browser/test/backend.test.ts

@@ -1,15 +1,7 @@
 import { expect } from 'chai';
-import { SentryEvent, SentryResponse, Status } from '../src';
+import { Status } from '../src';
 import { BrowserBackend } from '../src/backend';
-import { BaseTransport } from '../src/transports';
-
-class SimpleTransport extends BaseTransport {
-  public async captureEvent(_: SentryEvent): Promise<SentryResponse> {
-    return {
-      status: Status.fromHttpCode(200),
-    };
-  }
-}
+import { SimpleTransport } from './mocks/simpletransport';
 
 const dsn = 'https://[email protected]/42';
 const testEvent = {
@@ -34,7 +26,7 @@ describe('BrowserBackend', () => {
       }
     });
 
-    it('should call captureEvent() on provided transport', async () => {
+    it('should call sendEvent() on provided transport', async () => {
       backend = new BrowserBackend({ dsn, transport: SimpleTransport });
       const status = await backend.sendEvent(testEvent);
       expect(status.status).equal(Status.Success);

packages/browser/test/index.test.ts

@@ -1,8 +1,7 @@
 import { expect } from 'chai';
-import { SinonSpy, spy, stub } from 'sinon';
+import { SinonSpy, spy } from 'sinon';
 import {
   addBreadcrumb,
-  BrowserBackend,
   BrowserClient,
   captureEvent,
   captureException,
@@ -13,20 +12,21 @@ import {
   Integrations,
   Scope,
   SentryEvent,
-  Status,
 } from '../src';
+import { SimpleTransport } from './mocks/simpletransport';
 
 const dsn = 'https://[email protected]/4291';
 
 declare var global: any;
 
 describe('SentryBrowser', () => {
-  const beforeSend: SinonSpy = spy();
+  const beforeSend: SinonSpy = spy((event: SentryEvent) => event);
 
   before(() => {
     init({
       beforeSend,
       dsn,
+      transport: SimpleTransport,
     });
   });
 
@@ -69,16 +69,6 @@ describe('SentryBrowser', () => {
   });
 
   describe('breadcrumbs', () => {
-    let s: sinon.SinonStub;
-
-    beforeEach(() => {
-      s = stub(BrowserBackend.prototype, 'sendEvent').returns(Promise.resolve({ status: Status.Success }));
-    });
-
-    afterEach(() => {
-      s.restore();
-    });
-
     it('should record breadcrumbs', async () => {
       addBreadcrumb({ message: 'test1' });
       addBreadcrumb({ message: 'test2' });
@@ -90,16 +80,6 @@ describe('SentryBrowser', () => {
   });
 
   describe('capture', () => {
-    let s: sinon.SinonStub;
-
-    beforeEach(() => {
-      s = stub(BrowserBackend.prototype, 'sendEvent').returns(Promise.resolve({ status: Status.Success }));
-    });
-
-    afterEach(() => {
-      s.restore();
-    });
-
     it('should capture an exception', async () => {
       try {
         throw new Error('test');

packages/browser/test/integration/init.js

@@ -22,9 +22,9 @@ window.sentryBreadcrumbs = [];
 
 // stub transport so we don't actually transmit any data
 function DummyTransport() {}
-DummyTransport.prototype.captureEvent = function(event) {
+DummyTransport.prototype.sendEvent = function(event) {
   // console.log(JSON.stringify(event, null, 2));
-  sentryData.push(event);
+  sentryData.push(JSON.parse(event));
   done(sentryData);
   return Promise.resolve({
     status: 'success',

packages/browser/test/mocks/simpletransport.ts

@@ -0,0 +1,12 @@
+import { SentryResponse, Status } from '../../src';
+import { BaseTransport } from '../../src/transports';
+
+export class SimpleTransport extends BaseTransport {
+  public async sendEvent(_: string): Promise<SentryResponse> {
+    return this.buffer.add(
+      Promise.resolve({
+        status: Status.fromHttpCode(200),
+      }),
+    );
+  }
+}

packages/browser/test/transports/base.test.ts

@@ -6,13 +6,13 @@ const testDsn = 'https://[email protected]/42';
 class SimpleTransport extends BaseTransport {}
 
 describe('BaseTransport', () => {
-  it('doesnt provide captureEvent() implementation', async () => {
+  it('doesnt provide sendEvent() implementation', async () => {
     const transport = new SimpleTransport({ dsn: testDsn });
 
     try {
-      await transport.captureEvent({});
+      await transport.sendEvent('');
     } catch (e) {
-      expect(e.message).equal('Transport Class has to implement `captureEvent` method');
+      expect(e.message).equal('Transport Class has to implement `sendEvent` method');
     }
   });
 

packages/browser/test/transports/beacon.test.ts

@@ -29,11 +29,11 @@ describe('BeaconTransport', () => {
     expect(transport.url).equal(transportUrl);
   });
 
-  describe('captureEvent()', async () => {
+  describe('sendEvent()', async () => {
     it('sends a request to Sentry servers', async () => {
       sendBeacon.returns(true);
 
-      return transport.captureEvent(payload).then(res => {
+      return transport.sendEvent(JSON.stringify(payload)).then(res => {
         expect(res.status).equal(Status.Success);
         expect(sendBeacon.calledOnce).equal(true);
         expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true);
@@ -43,7 +43,7 @@ describe('BeaconTransport', () => {
     it('rejects with failed status', async () => {
       sendBeacon.returns(false);
 
-      return transport.captureEvent(payload).catch(res => {
+      return transport.sendEvent(JSON.stringify(payload)).catch(res => {
         expect(res.status).equal(Status.Failed);
         expect(sendBeacon.calledOnce).equal(true);
         expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true);