Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct removeAllListeners in case no event is passed #2088

Merged
merged 4 commits into from
Apr 9, 2021
Merged

fix: correct removeAllListeners in case no event is passed #2088

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
057e9d9
fix: correct removeAllListeners in case no event is passed
Flarna Apr 8, 2021
ab8683a
fixup! use strict equal and Object.create(null)
Flarna Apr 8, 2021
df0bc39
Merge branch 'main' into remove-all-listeners
Flarna Apr 9, 2021
dd9e1bf
Merge branch 'main' into remove-all-listeners
vmarchaud Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,14 @@ export abstract class AbstractAsyncHooksContextManager
const contextManager = this;
return function (this: never, event: string) {
const map = contextManager._getPatchMap(ee);
if (map?.[event] !== undefined) {
delete map[event];
if (map !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const map = null; 
map !== undefined; 

map['aa'] !== undefined; 
// error
Suggested change
if (map !== undefined) {
if (map) {

Copy link
Member Author

@Flarna Flarna Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used !== undefined here because it's used at a lot places in this file. Usually I prefer != null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if the map is null then the condition will pass and you will have undefined object exception

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!= null would catch both null and undefined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the map will never be null in this case, only undefined. If a dev made a change which allowed this to be null, the test would fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that map is never null here, i don't see the benefit of checking for both undefined and null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said I usually use != null. But there are a lot places in this file where !== undefined is already used so the no 1 rule 'When in Rome, Do as the Romans Do' wins.
I would prefer to do cleanups like this in a non bugfix PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well if (map) { would simply do the trick and will be a bullet proof for any future changes, you don't need to use != null

if (arguments.length === 0) {
contextManager._createPatchMap(ee);
} else if (map[event] !== undefined) {
delete map[event];
}
}
return original.call(this, event);
return original.apply(this, arguments);
};
}

Expand Down Expand Up @@ -184,7 +188,7 @@ export abstract class AbstractAsyncHooksContextManager
}

private _createPatchMap(ee: EventEmitter): PatchMap {
const map = {};
const map = Object.create(null);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(ee as any)[this._kOtListeners] = map;
return map;
Expand Down
18 changes: 18 additions & 0 deletions packages/opentelemetry-context-async-hooks/test/AsyncHooksContextManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,24 @@ for (const contextManagerClass of [
patchedEE.emit('test');
});

it('should return current context and removeAllListeners (when enabled)', done => {
const ee = new EventEmitter();
const context = ROOT_CONTEXT.setValue(key1, 1);
const patchedEE = contextManager.bind(ee, context);
const handler = () => {
assert.deepStrictEqual(contextManager.active(), context);
patchedEE.removeAllListeners();
assert.strictEqual(patchedEE.listeners('test').length, 0);
assert.strictEqual(patchedEE.listeners('test1').length, 0);
return done();
};
patchedEE.on('test', handler);
patchedEE.on('test1', handler);
assert.strictEqual(patchedEE.listeners('test').length, 1);
assert.strictEqual(patchedEE.listeners('test1').length, 1);
patchedEE.emit('test');
});

/**
* Even if asynchooks is disabled, the context propagation will
* still works but it might be lost after any async op.
Expand Down