fix: Hub domain handling and requestHandler/errorHandler updates

Author: kamilogorek

CHANGELOG.md

@@ -24,10 +24,14 @@
 - [core] ref: Remove Repo interface and repos attribute from Event
 - [core] ref: Rewrite RequestBuffer using Array instead of Set for IE10/11
 - [hub] fix: Scope level overwrites level on the event
+- [hub] fix: Correctly store and retrieve Hub from domain when one is active
+- [hub] fix: Copy over user data when cloning scope
 - [node] feat: Allow requestHandler to be configured
-- [node] feat: Make node transactions a pluggable integration with tests 
+- [node] feat: Make node transactions a pluggable integration with tests
 - [node] feat: Transactions handling for RequestHandler in Express/Hapi
 - [node] fix: Dont wrap native modules more than once to prevent leaks
+- [node] fix: Use getCurrentHub to retrieve correct hub in requestHandler
+
 - [utils] ref: implemented includes, assign and isNaN polyfills
 
 ## 4.0.6

packages/browser/rollup.config.js

@@ -1,9 +1,9 @@
-import builtins from 'rollup-plugin-node-builtins';
 import commonjs from 'rollup-plugin-commonjs';
 import uglify from 'rollup-plugin-uglify';
 import resolve from 'rollup-plugin-node-resolve';
 import typescript from 'rollup-plugin-typescript2';
 import license from 'rollup-plugin-license';
+import builtins from 'rollup-plugin-node-builtins';
 
 const commitHash = require('child_process')
   .execSync('git rev-parse --short HEAD', { encoding: 'utf-8' })

packages/core/test/integrations/rewriteframes.test.ts renamed to packages/core/test/lib/integrations/rewriteframes.test.ts

@@ -1,5 +1,5 @@
 import { SentryEvent, StackFrame } from '@sentry/types';
-import { RewriteFrames } from '../../src/integrations/pluggable/rewriteframes';
+import { RewriteFrames } from '../../../src/integrations/pluggable/rewriteframes';
 
 let rewriteFrames: RewriteFrames;
 let messageEvent: SentryEvent;

packages/hub/src/hub.ts

@@ -287,6 +287,11 @@ export function makeMain(hub?: Hub): Hub | undefined {
   return oldHub;
 }
 
+/** Domain interface with attached Hub */
+interface SentryDomain extends NodeJS.Domain {
+  __SENTRY__?: Carrier;
+}
+
 /**
  * Returns the default hub instance.
  *
@@ -301,7 +306,28 @@ export function getCurrentHub(): Hub {
     registry.hub = new Hub();
   }
 
-  return registry.hub;
+  let domain = null;
+  try {
+    domain = process.domain as SentryDomain;
+  } catch (_Oo) {
+    // We do not have process
+  }
+
+  if (!domain) {
+    return registry.hub;
+  }
+
+  let carrier = domain.__SENTRY__;
+  if (!carrier) {
+    domain.__SENTRY__ = carrier = {};
+  }
+
+  if (!carrier.hub) {
+    const top = registry.hub.getStackTop();
+    carrier.hub = top ? new Hub(top.client, Scope.clone(top.scope)) : new Hub();
+  }
+
+  return carrier.hub;
 }
 
 /**

packages/hub/src/scope.ts

@@ -139,6 +139,9 @@ export class Scope {
       scopeListeners: [],
     });
     if (scope) {
+      newScope.extra = assign(scope.extra);
+      newScope.tags = assign(scope.tags) as any;
+      newScope.breadcrumbs = [...scope.breadcrumbs];
       newScope.eventProcessors = [...scope.eventProcessors];
     }
     return newScope;

packages/node/src/handlers.ts

@@ -1,5 +1,5 @@
 import { logger } from '@sentry/core';
-import { getHubFromCarrier, Scope } from '@sentry/hub';
+import { getCurrentHub, Scope } from '@sentry/hub';
 import { SentryEvent, Severity } from '@sentry/types';
 import { forget } from '@sentry/utils/async';
 import { serialize } from '@sentry/utils/object';
@@ -9,7 +9,6 @@ import * as http from 'http';
 import * as os from 'os';
 import * as url from 'url';
 import { NodeClient } from './client';
-import { getCurrentHub } from './hub';
 
 const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
 
@@ -208,20 +207,22 @@ export function requestHandler(options?: {
   transaction?: boolean | TransactionTypes;
   user?: boolean;
   version?: boolean;
-}): (req: http.IncomingMessage, res: http.ServerResponse, next: () => void) => void {
+}): (req: http.IncomingMessage, res: http.ServerResponse, next: (error?: any) => void) => void {
   return function sentryRequestMiddleware(
     req: http.IncomingMessage,
-    _res: http.ServerResponse,
-    next: () => void,
+    res: http.ServerResponse,
+    next: (error?: any) => void,
   ): void {
     const local = domain.create();
-    const hub = getHubFromCarrier(req);
-    hub.bindClient(getCurrentHub().getClient());
-    hub.configureScope((scope: Scope) => {
-      scope.addEventProcessor(async (event: SentryEvent) => parseRequest(event, req, options));
-    });
+    local.add(req);
+    local.add(res);
     local.on('error', next);
-    local.run(next);
+    local.run(() => {
+      getCurrentHub().configureScope(scope =>
+        scope.addEventProcessor(async (event: SentryEvent) => parseRequest(event, req, options)),
+      );
+      next();
+    });
   };
 }
 
@@ -238,7 +239,6 @@ interface MiddlewareError extends Error {
 /** JSDoc */
 function getStatusCodeFromResponse(error: MiddlewareError): number {
   const statusCode = error.status || error.statusCode || error.status_code || (error.output && error.output.statusCode);
-
   return statusCode ? parseInt(statusCode as string, 10) : 500;
 }
 
@@ -251,7 +251,7 @@ export function errorHandler(): (
 ) => void {
   return function sentryErrorMiddleware(
     error: MiddlewareError,
-    req: http.IncomingMessage,
+    _req: http.IncomingMessage,
     _res: http.ServerResponse,
     next: (error: MiddlewareError) => void,
   ): void {
@@ -260,7 +260,7 @@ export function errorHandler(): (
       next(error);
       return;
     }
-    getHubFromCarrier(req).captureException(error, { originalException: error });
+    getCurrentHub().captureException(error, { originalException: error });
     next(error);
   };
 }

packages/node/src/hub.ts

@@ -19,13 +19,13 @@ export {
   captureEvent,
   captureMessage,
   configureScope,
+  getCurrentHub,
   getHubFromCarrier,
   Hub,
   Scope,
   withScope,
 } from '@sentry/core';
 
-export { getCurrentHub } from './hub';
 export { NodeBackend, NodeOptions } from './backend';
 export { NodeClient } from './client';
 export { defaultIntegrations, init } from './sdk';

packages/node/src/index.ts

@@ -1,7 +1,7 @@
+import { getCurrentHub } from '@sentry/hub';
 import { Integration, Severity } from '@sentry/types';
 import { fill } from '@sentry/utils/object';
 import * as util from 'util';
-import { getCurrentHub } from '../hub';
 
 /**
  * Wrapper function for internal _load calls within `require`

packages/node/src/integrations/console.ts

@@ -1,8 +1,8 @@
+import { getCurrentHub } from '@sentry/hub';
 import { Integration } from '@sentry/types';
 import { fill } from '@sentry/utils/object';
 import * as http from 'http';
 import * as util from 'util';
-import { getCurrentHub } from '../hub';
 
 let lastResponse: http.ServerResponse | undefined;
 

packages/node/src/integrations/http.ts

@@ -5,3 +5,4 @@ export { OnUnhandledRejection } from './onunhandledrejection';
 export { LinkedErrors } from './linkederrors';
 
 export { Modules } from './pluggable/modules';
+export { Transaction } from './pluggable/transaction';

packages/node/src/integrations/index.ts

@@ -1,5 +1,5 @@
+import { getCurrentHub } from '@sentry/hub';
 import { Integration, SentryEvent, SentryEventHint, SentryException } from '@sentry/types';
-import { getCurrentHub } from '../hub';
 import { getExceptionFromError } from '../parsers';
 
 const DEFAULT_KEY = 'cause';

packages/node/src/integrations/linkederrors.ts

@@ -1,5 +1,5 @@
+import { getCurrentHub } from '@sentry/hub';
 import { Integration } from '@sentry/types';
-import { getCurrentHub } from '../hub';
 
 /** Global Promise Rejection handler */
 export class OnUnhandledRejection implements Integration {

packages/node/src/integrations/onunhandledrejection.ts

@@ -1,8 +1,7 @@
-import { Scope } from '@sentry/hub';
+import { getCurrentHub, Scope } from '@sentry/hub';
 import { Integration } from '@sentry/types';
 import * as lsmod from 'lsmod';
 import { NodeOptions } from '../../backend';
-import { getCurrentHub } from '../../hub';
 
 let moduleCache: { [key: string]: string };
 

packages/node/src/integrations/pluggable/modules.ts

@@ -1,7 +1,6 @@
-import { Scope } from '@sentry/hub';
+import { getCurrentHub, Scope } from '@sentry/hub';
 import { Integration, SentryEvent, StackFrame } from '@sentry/types';
 import { NodeOptions } from '../../backend';
-import { getCurrentHub } from '../../hub';
 
 /** Add node transaction to the event */
 export class Transaction implements Integration {

packages/node/src/integrations/pluggable/transaction.ts

@@ -1,39 +1,7 @@
-import { Layer } from '@sentry/hub';
+import { getCurrentHub, Hub } from '@sentry/hub';
 import * as domain from 'domain';
 
-const mockgetCurrentHub = jest.fn();
-
-class MockHub {
-  public constructor(public stack: Layer[] = []) {
-    MockHub.instance = this;
-  }
-  public static instance: MockHub;
-
-  public getStack(): Layer[] {
-    return this.stack;
-  }
-
-  public getStackTop(): Layer {
-    return this.stack[this.stack.length - 1];
-  }
-}
-
-const mockHub = MockHub;
-jest.mock('@sentry/hub', () => ({
-  Hub: mockHub,
-  getCurrentHub: mockgetCurrentHub,
-}));
-
-import { getCurrentHub } from '../src';
-
 describe('domains', () => {
-  let globalHub: MockHub;
-
-  beforeEach(() => {
-    globalHub = new MockHub();
-    mockgetCurrentHub.mockReturnValue(globalHub);
-  });
-
   afterEach(() => {
     if (domain.active) {
       domain.active.exit();
@@ -44,26 +12,20 @@ describe('domains', () => {
   test('without domain', () => {
     expect(domain.active).toBeFalsy();
     const hub = getCurrentHub();
-    expect(hub).toBe(globalHub);
+    expect(hub).toEqual(new Hub());
   });
 
-  test('domain hub inheritance', () => {
-    globalHub.stack = [];
-    const d = domain.create();
-    d.run(() => {
-      const hub = getCurrentHub();
-      expect(globalHub).not.toBe(hub);
-      expect(globalHub.getStack()).toEqual(hub.getStack());
+  test('domain hub scope inheritance', () => {
+    const globalHub = getCurrentHub();
+    globalHub.configureScope(scope => {
+      scope.setExtra('a', 'b');
+      scope.setTag('a', 'b');
+      scope.addBreadcrumb({ message: 'a' });
     });
-  });
-
-  test('domain hub isolation', () => {
     const d = domain.create();
     d.run(() => {
       const hub = getCurrentHub();
-      hub.getStack().push({ client: 'whatever' });
-      expect(hub.getStack()).toEqual([{ client: 'whatever' }]);
-      expect(globalHub.getStack()).toEqual([]);
+      expect(globalHub).toEqual(hub);
     });
   });
 
@@ -84,7 +46,7 @@ describe('domains', () => {
         .push({ client: 'process' });
 
       setTimeout(() => {
-        expect(getCurrentHub().getStack()).toEqual([{ client: 'process' }]);
+        expect(getCurrentHub().getStack()[1]).toEqual({ client: 'process' });
       }, 50);
     });
 
@@ -94,7 +56,7 @@ describe('domains', () => {
         .push({ client: 'local' });
 
       setTimeout(() => {
-        expect(getCurrentHub().getStack()).toEqual([{ client: 'local' }]);
+        expect(getCurrentHub().getStack()[1]).toEqual({ client: 'local' });
         done();
       }, 100);
     });

packages/node/test/domain.test.ts

@@ -27,7 +27,7 @@
   },
   "scripts": {
     "build": "run-s clean; tsc -p tsconfig.build.json",
-    "build:watch": "run-s clean; tsc -p tsconfig.build.json -w --preserveWatchOutput",
+    "build:watch": "tsc -p tsconfig.build.json -w --preserveWatchOutput",
     "clean": "rimraf dist coverage *.js *.js.map *.d.ts",
     "lint": "run-s lint:prettier lint:tslint",
     "lint:prettier": "prettier-check '{src,test}/**/*.ts'",