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.

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

Detect if listener is already wrapped by once #3122

Closed
wants to merge 4 commits into from

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jul 28, 2022

Fixes #2971

This is an alternative solution to #3123

@dyladan dyladan requested a review from a team July 28, 2022 22:47
@dyladan dyladan added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc labels Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #3122 (316ccae) into main (df58fac) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3122      +/-   ##
==========================================
+ Coverage   93.07%   93.21%   +0.14%     
==========================================
  Files         195      186       -9     
  Lines        6384     5838     -546     
  Branches     1347     1252      -95     
==========================================
- Hits         5942     5442     -500     
+ Misses        442      396      -46     
Impacted Files Coverage Δ
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 97.10% <100.00%> (+0.08%) ⬆️
...y-instrumentation-grpc/src/enums/AttributeNames.ts
...entelemetry-instrumentation-grpc/src/grpc/index.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
...metry-instrumentation-grpc/src/grpc/clientUtils.ts
...es/opentelemetry-instrumentation-grpc/src/utils.ts
...elemetry-instrumentation-grpc/src/grpc-js/index.ts
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts
...emetry-instrumentation-grpc/src/instrumentation.ts
...metry-instrumentation-grpc/src/grpc/serverUtils.ts

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

I prefer this solution over #3123 since it only change patch for add listener (whereas the other one modify the bind which is much more used).

@@ -177,6 +177,11 @@ implements ContextManager {
) {
const contextManager = this;
return function (this: never, event: string, listener: Func<void>) {
// @ts-expect-error listener is not a property of type Function but it is actually used by the onceWrapper in events.js
if (typeof listener === 'function' && typeof listener.listener === 'function' && listener.name === 'bound onceWrapper') {
Copy link
Member

Choose a reason for hiding this comment

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

be aware that OTel context manager is likely not be the only one patching EventEmitter. More or less any APM product and a lot other tools (e.g. zone.js - yes it's used by node.js apps not only browsers) patch this also.

Therefore you might have others between once and on.

You could rely on the single threaded nature of JS and set some inOnce flag in ContextManager and check this one instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could rely on the single threaded nature of JS and set some inOnce flag in ContextManager and check this one instead.

Good idea. Opened a third PR #3132

@dyladan
Copy link
Member Author

dyladan commented Aug 1, 2022

I think I prefer #3133. Closing this but it can be reopened if someone disagrees.

@dyladan dyladan closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbstractAsyncHooksContextManager breaks .removeListener for handlers which was added with .once
3 participants