From 2bae628db644024ba53cf1c789adab21aeb58866 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Thu, 28 Jul 2022 18:41:57 -0400 Subject: [PATCH 1/6] Add failing tests for #2971 --- .../test/AsyncHooksContextManager.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts index 0b8e4868919..5f61505c521 100644 --- a/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts +++ b/packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts @@ -415,6 +415,28 @@ for (const contextManagerClass of [ patchedEE.emit('test'); }); + it('should remove event handler enabled by .once using removeListener (when enabled)', () => { + const ee = new EventEmitter(); + const context = ROOT_CONTEXT.setValue(key1, 1); + const patchedEE = contextManager.bind(context, ee); + function handler() {} + patchedEE.once('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.removeListener('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 0); + }); + + it('should remove event handler enabled by .once using off (when enabled)', () => { + const ee = new EventEmitter(); + const context = ROOT_CONTEXT.setValue(key1, 1); + const patchedEE = contextManager.bind(context, ee); + const handler = () => { }; + patchedEE.once('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 1); + patchedEE.off('test', handler); + assert.strictEqual(patchedEE.listeners('test').length, 0); + }); + it('should return current context and removeAllListeners (when enabled)', done => { const ee = new EventEmitter(); const context = ROOT_CONTEXT.setValue(key1, 1); From ad4fa5ce44aae769e73a5e2d6149bcec68ab0561 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Mon, 1 Aug 2022 09:45:29 -0400 Subject: [PATCH 2/6] Prevent double wrapping event listeners --- CHANGELOG.md | 3 +++ .../src/AbstractAsyncHooksContextManager.ts | 13 +++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91171078898..c972ad4724b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ All notable changes to this project will be documented in this file. ### :bug: (Bug Fix) +* fix(context-async-hooks): Ensure listeners added using `once` can be removed using `removeListener` + [#3133](https://github.com/open-telemetry/opentelemetry-js/pull/3133) + ### :books: (Refine Doc) ### :house: (Internal) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index 7c8748f2589..965d6de2895 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -37,7 +37,7 @@ const ADD_LISTENER_METHODS = [ ]; export abstract class AbstractAsyncHooksContextManager -implements ContextManager { + implements ContextManager { abstract active(): Context; abstract with ReturnType>( @@ -177,6 +177,10 @@ implements ContextManager { ) { const contextManager = this; return function (this: never, event: string, listener: Func) { + // do not double wrap. e.g. once calls on + if (contextManager._wrapped) { + return original.call(this, event, listener); + } let map = contextManager._getPatchMap(ee); if (map === undefined) { map = contextManager._createPatchMap(ee); @@ -189,7 +193,11 @@ implements ContextManager { const patchedListener = contextManager.bind(context, listener); // store a weak reference of the user listener to ours listeners.set(listener, patchedListener); - return original.call(this, event, patchedListener); + + contextManager._wrapped = true; + const val = original.call(this, event, patchedListener); + contextManager._wrapped = false; + return val; }; } @@ -204,4 +212,5 @@ implements ContextManager { } private readonly _kOtListeners = Symbol('OtListeners'); + private _wrapped = false; } From 654de54031629e1ee764f18bc2e0db7de6cbfa72 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Mon, 1 Aug 2022 09:47:31 -0400 Subject: [PATCH 3/6] Protect against listener throw --- .../src/AbstractAsyncHooksContextManager.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index 965d6de2895..e6229a0a60c 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -195,9 +195,11 @@ export abstract class AbstractAsyncHooksContextManager listeners.set(listener, patchedListener); contextManager._wrapped = true; - const val = original.call(this, event, patchedListener); - contextManager._wrapped = false; - return val; + try { + return original.call(this, event, patchedListener);; + } finally { + contextManager._wrapped = false; + } }; } From ff643b52bd4b2295f94677fd78c19fd491a8cd9c Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Mon, 1 Aug 2022 09:48:29 -0400 Subject: [PATCH 4/6] Lint --- .../src/AbstractAsyncHooksContextManager.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index e6229a0a60c..5dcaa08a02b 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -37,7 +37,7 @@ const ADD_LISTENER_METHODS = [ ]; export abstract class AbstractAsyncHooksContextManager - implements ContextManager { +implements ContextManager { abstract active(): Context; abstract with ReturnType>( @@ -193,10 +193,10 @@ export abstract class AbstractAsyncHooksContextManager const patchedListener = contextManager.bind(context, listener); // store a weak reference of the user listener to ours listeners.set(listener, patchedListener); - + contextManager._wrapped = true; try { - return original.call(this, event, patchedListener);; + return original.call(this, event, patchedListener); } finally { contextManager._wrapped = false; } From 79e06ac4d1ee3ce9d92777905141568c943599b1 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 2 Aug 2022 10:10:23 -0400 Subject: [PATCH 5/6] Add comment explanation for onceWrapper detection --- .../src/AbstractAsyncHooksContextManager.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index 5dcaa08a02b..e3c450e9415 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -177,7 +177,14 @@ implements ContextManager { ) { const contextManager = this; return function (this: never, event: string, listener: Func) { - // do not double wrap. e.g. once calls on + /** + * This check is required to prevent double-wrapping the listener. + * The implementation for ee.once wraps the listener and calls ee.on. + * Without this check, we would wrap that wrapped listener. + * This causes an issue because ee.removeListener depends on the onceWrapper + * to properly remove the listener. If we wrap their wrapper, we break + * that detection. + */ if (contextManager._wrapped) { return original.call(this, event, listener); } @@ -194,6 +201,9 @@ implements ContextManager { // store a weak reference of the user listener to ours listeners.set(listener, patchedListener); + /** + * See comment at the start of this function for the explanation of this property. + */ contextManager._wrapped = true; try { return original.call(this, event, patchedListener); From 035b7d3b9d0f39f67aa6b81bce021fa880773b98 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 2 Aug 2022 10:55:09 -0400 Subject: [PATCH 6/6] lint --- .../src/AbstractAsyncHooksContextManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts index e3c450e9415..4a46f634f58 100644 --- a/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts +++ b/packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts @@ -201,7 +201,7 @@ implements ContextManager { // store a weak reference of the user listener to ours listeners.set(listener, patchedListener); - /** + /** * See comment at the start of this function for the explanation of this property. */ contextManager._wrapped = true;