From 1d846bb0a61defa030e6e72adb63eae1554ca083 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Tue, 16 Feb 2021 19:46:49 +0100 Subject: [PATCH 1/4] fix: isolate binding EventEmitter Binding an EventEmitter a second time via another instances of ContextManager shouldn't have side effects to the first ContextManager. Use an unique symbol per ContextManager instance to isolate them. --- .../src/AbstractAsyncHooksContextManager.ts | 75 +++++++++++-------- .../src/AsyncLocalStorageContextManager.ts | 2 +- .../test/AsyncHooksContextManager.test.ts | 24 ++++++ 3 files changed, 69 insertions(+), 32 deletions(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index 85bf9c5dd6f..dad46aff906 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -17,18 +17,16 @@ import { ContextManager, Context } from '@opentelemetry/context-base'; import { EventEmitter } from 'events'; -const kOtListeners = Symbol('OtListeners'); - type Func = (...args: unknown[]) => T; -type PatchedEventEmitter = { - /** - * Store a map for each event of all original listener and their "patched" - * version so when the listener is removed by the user, we remove the - * corresponding "patched" function. - */ - [kOtListeners]?: { [name: string]: WeakMap, Func> }; -} & EventEmitter; +/** + * Store a map for each event of all original listener and their "patched" + * version so when the listener is removed by the user, we remove the + * corresponding "patched" function. + */ +interface PatchMap { + [name: string]: WeakMap, Func>; +} const ADD_LISTENER_METHODS = [ 'addListener' as const, @@ -66,7 +64,7 @@ export abstract class AbstractAsyncHooksContextManager private _bindFunction(target: T, context: Context): T { const manager = this; - const contextWrapper = function (this: {}, ...args: unknown[]) { + const contextWrapper = function (this: never, ...args: unknown[]) { return manager.with(context, () => target.apply(this, args)); }; Object.defineProperty(contextWrapper, 'length', { @@ -91,12 +89,12 @@ export abstract class AbstractAsyncHooksContextManager * @param context the context we want to bind */ private _bindEventEmitter( - target: T, + ee: T, context: Context ): T { - const ee = (target as unknown) as PatchedEventEmitter; - if (ee[kOtListeners] !== undefined) return target; - ee[kOtListeners] = {}; + const map = this._getPatchMap(ee); + if (map !== undefined) return ee; + this._createPatchMap(ee); // patch methods that add a listener to propagate context ADD_LISTENER_METHODS.forEach(methodName => { @@ -117,7 +115,7 @@ export abstract class AbstractAsyncHooksContextManager ee.removeAllListeners ); } - return target; + return ee; } /** @@ -126,9 +124,10 @@ export abstract class AbstractAsyncHooksContextManager * @param ee EventEmitter instance * @param original reference to the patched method */ - private _patchRemoveListener(ee: PatchedEventEmitter, original: Function) { - return function (this: {}, event: string, listener: Func) { - const events = ee[kOtListeners]?.[event]; + private _patchRemoveListener(ee: EventEmitter, original: Function) { + const contextManager = this; + return function (this: never, event: string, listener: Func) { + const events = contextManager._getPatchMap(ee)?.[event]; if (events === undefined) { return original.call(this, event, listener); } @@ -143,13 +142,12 @@ export abstract class AbstractAsyncHooksContextManager * @param ee EventEmitter instance * @param original reference to the patched method */ - private _patchRemoveAllListeners( - ee: PatchedEventEmitter, - original: Function - ) { - return function (this: {}, event: string) { - if (ee[kOtListeners]?.[event] !== undefined) { - delete ee[kOtListeners]![event]; + private _patchRemoveAllListeners(ee: EventEmitter, original: Function) { + const contextManager = this; + return function (this: never, event: string) { + const map = contextManager._getPatchMap(ee); + if (map?.[event] !== undefined) { + delete map[event]; } return original.call(this, event); }; @@ -163,17 +161,20 @@ export abstract class AbstractAsyncHooksContextManager * @param [context] context to propagate when calling listeners */ private _patchAddListener( - ee: PatchedEventEmitter, + ee: EventEmitter, original: Function, context: Context ) { const contextManager = this; - return function (this: {}, event: string, listener: Func) { - if (ee[kOtListeners] === undefined) ee[kOtListeners] = {}; - let listeners = ee[kOtListeners]![event]; + return function (this: never, event: string, listener: Func) { + let map = contextManager._getPatchMap(ee); + if (map === undefined) { + map = contextManager._createPatchMap(ee); + } + let listeners = map[event]; if (listeners === undefined) { listeners = new WeakMap(); - ee[kOtListeners]![event] = listeners; + map[event] = listeners; } const patchedListener = contextManager.bind(listener, context); // store a weak reference of the user listener to ours @@ -181,4 +182,16 @@ export abstract class AbstractAsyncHooksContextManager return original.call(this, event, patchedListener); }; } + + private _createPatchMap(ee: EventEmitter): PatchMap { + const map = {}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (ee as any)[this._kOtListeners] = map; + return map; + } + private _getPatchMap(ee: EventEmitter): PatchMap | undefined { + return (ee as never)[this._kOtListeners]; + } + + private readonly _kOtListeners = Symbol('OtListeners'); } diff --git a/packages/opentelemetry-context-async-hooks/src/AsyncLocalStorageContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AsyncLocalStorageContextManager.ts index 87cd0941f54..4a4aa29f2ba 100644 --- a/packages/opentelemetry-context-async-hooks/src/AsyncLocalStorageContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AsyncLocalStorageContextManager.ts @@ -37,7 +37,7 @@ export class AsyncLocalStorageContextManager extends AbstractAsyncHooksContextMa ...args: A ): ReturnType { const cb = thisArg == null ? fn : fn.bind(thisArg); - return this._asyncLocalStorage.run(context, cb as any, ...args); + return this._asyncLocalStorage.run(context, cb as never, ...args); } enable(): this { diff --git a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts index 32921f71c26..ee3cc295883 100644 --- a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts +++ b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts @@ -31,6 +31,9 @@ for (const contextManagerClass of [ | AsyncHooksContextManager | AsyncLocalStorageContextManager; const key1 = createContextKey('test key 1'); + let otherContextManager: + | AsyncHooksContextManager + | AsyncLocalStorageContextManager; before(function () { if ( @@ -49,6 +52,7 @@ for (const contextManagerClass of [ afterEach(() => { contextManager.disable(); + otherContextManager?.disable(); }); describe('.enable()', () => { @@ -416,6 +420,26 @@ for (const contextManagerClass of [ assert.strictEqual(patchedEe.listeners('test').length, 1); patchedEe.emit('test'); }); + + it('should not influence other instances', () => { + const ee = new EventEmitter(); + otherContextManager = new contextManagerClass(); + otherContextManager.enable(); + + const context = ROOT_CONTEXT.setValue(key1, 2); + const otherContext = ROOT_CONTEXT.setValue(key1, 3); + const patchedEe = otherContextManager.bind( + contextManager.bind(ee, context), + otherContext + ); + const handler = () => { + assert.deepStrictEqual(contextManager.active(), context); + assert.strictEqual(otherContextManager.active(), otherContext); + }; + + patchedEe.on('test', handler); + patchedEe.emit('test'); + }); }); }); } From 099c115e9c5fb2c2d45073d5f9d65be1b0532a3c Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Wed, 17 Feb 2021 23:20:13 +0100 Subject: [PATCH 2/4] chore: add testcases for other APIs --- .../test/AsyncHooksContextManager.test.ts | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts index ee3cc295883..b54b5913f0e 100644 --- a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts +++ b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts @@ -278,6 +278,22 @@ for (const contextManagerClass of [ countDown(); }, time2); }); + + it('should not influence other instances', () => { + otherContextManager = new contextManagerClass(); + otherContextManager.enable(); + + const context = ROOT_CONTEXT.setValue(key1, 2); + const otherContext = ROOT_CONTEXT.setValue(key1, 3); + contextManager.with(context, () => { + assert.strictEqual(contextManager.active(), context); + assert.strictEqual(otherContextManager.active(), ROOT_CONTEXT); + otherContextManager.with(otherContext, () => { + assert.strictEqual(contextManager.active(), context); + assert.strictEqual(otherContextManager.active(), otherContext); + }); + }); + }); }); describe('.bind(function)', () => { @@ -339,6 +355,22 @@ for (const contextManagerClass of [ }, context); fn(); }); + + it('should not influence other instances', () => { + otherContextManager = new contextManagerClass(); + otherContextManager.enable(); + + const context = ROOT_CONTEXT.setValue(key1, 2); + const otherContext = ROOT_CONTEXT.setValue(key1, 3); + const fn = otherContextManager.bind( + contextManager.bind(() => { + assert.strictEqual(contextManager.active(), context); + assert.strictEqual(otherContextManager.active(), otherContext); + }, context), + otherContext + ); + fn(); + }); }); describe('.bind(event-emitter)', () => { @@ -433,7 +465,7 @@ for (const contextManagerClass of [ otherContext ); const handler = () => { - assert.deepStrictEqual(contextManager.active(), context); + assert.strictEqual(contextManager.active(), context); assert.strictEqual(otherContextManager.active(), otherContext); }; From 877a8e776246bad767df77e34b42eb4ff6e4150e Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Wed, 17 Feb 2021 23:52:53 +0100 Subject: [PATCH 3/4] chore: fix review findings --- .../src/AbstractAsyncHooksContextManager.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index dad46aff906..950b47acf3e 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -20,9 +20,9 @@ import { EventEmitter } from 'events'; type Func = (...args: unknown[]) => T; /** - * Store a map for each event of all original listener and their "patched" - * version so when the listener is removed by the user, we remove the - * corresponding "patched" function. + * Store a map for each event of all original listeners and their "patched" + * version. So when a listener is removed by the user, the corresponding + * patched function will be also removed. */ interface PatchMap { [name: string]: WeakMap, Func>; @@ -85,7 +85,7 @@ export abstract class AbstractAsyncHooksContextManager * By default, EventEmitter call their callback with their context, which we do * not want, instead we will bind a specific context to all callbacks that * go through it. - * @param target EventEmitter a instance of EventEmitter to patch + * @param ee EventEmitter an instance of EventEmitter to patch * @param context the context we want to bind */ private _bindEventEmitter( From c21a21a6b7147a2dd3ff072debd811213c80a6ca Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Thu, 18 Feb 2021 07:43:59 +0100 Subject: [PATCH 4/4] chore: patchedEe => patchedEE --- .../test/AsyncHooksContextManager.test.ts | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts index b54b5913f0e..2ef5b887996 100644 --- a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts +++ b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts @@ -388,31 +388,31 @@ for (const contextManagerClass of [ it('should return current context and removeListener (when enabled)', done => { const ee = new EventEmitter(); const context = ROOT_CONTEXT.setValue(key1, 1); - const patchedEe = contextManager.bind(ee, context); + const patchedEE = contextManager.bind(ee, context); const handler = () => { assert.deepStrictEqual(contextManager.active(), context); - patchedEe.removeListener('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 0); + patchedEE.removeListener('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 0); return done(); }; - patchedEe.on('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 1); - patchedEe.emit('test'); + patchedEE.on('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.emit('test'); }); it('should return current context and removeAllListener (when enabled)', done => { const ee = new EventEmitter(); const context = ROOT_CONTEXT.setValue(key1, 1); - const patchedEe = contextManager.bind(ee, context); + const patchedEE = contextManager.bind(ee, context); const handler = () => { assert.deepStrictEqual(contextManager.active(), context); - patchedEe.removeAllListeners('test'); - assert.strictEqual(patchedEe.listeners('test').length, 0); + patchedEE.removeAllListeners('test'); + assert.strictEqual(patchedEE.listeners('test').length, 0); return done(); }; - patchedEe.on('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 1); - patchedEe.emit('test'); + patchedEE.on('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.emit('test'); }); /** @@ -423,34 +423,34 @@ for (const contextManagerClass of [ contextManager.disable(); const ee = new EventEmitter(); const context = ROOT_CONTEXT.setValue(key1, 1); - const patchedEe = contextManager.bind(ee, context); + const patchedEE = contextManager.bind(ee, context); const handler = () => { assert.deepStrictEqual(contextManager.active(), context); - patchedEe.removeListener('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 0); + patchedEE.removeListener('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 0); return done(); }; - patchedEe.on('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 1); - patchedEe.emit('test'); + patchedEE.on('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.emit('test'); }); it('should not return current context with async op', done => { const ee = new EventEmitter(); const context = ROOT_CONTEXT.setValue(key1, 1); - const patchedEe = contextManager.bind(ee, context); + const patchedEE = contextManager.bind(ee, context); const handler = () => { assert.deepStrictEqual(contextManager.active(), context); setImmediate(() => { assert.deepStrictEqual(contextManager.active(), context); - patchedEe.removeAllListeners('test'); - assert.strictEqual(patchedEe.listeners('test').length, 0); + patchedEE.removeAllListeners('test'); + assert.strictEqual(patchedEE.listeners('test').length, 0); return done(); }); }; - patchedEe.on('test', handler); - assert.strictEqual(patchedEe.listeners('test').length, 1); - patchedEe.emit('test'); + patchedEE.on('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.emit('test'); }); it('should not influence other instances', () => { @@ -460,7 +460,7 @@ for (const contextManagerClass of [ const context = ROOT_CONTEXT.setValue(key1, 2); const otherContext = ROOT_CONTEXT.setValue(key1, 3); - const patchedEe = otherContextManager.bind( + const patchedEE = otherContextManager.bind( contextManager.bind(ee, context), otherContext ); @@ -469,8 +469,8 @@ for (const contextManagerClass of [ assert.strictEqual(otherContextManager.active(), otherContext); }; - patchedEe.on('test', handler); - patchedEe.emit('test'); + patchedEE.on('test', handler); + patchedEE.emit('test'); }); }); });