Skip to content

fix: do not add abort signals to "useless" addresses #913

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

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

vasco-santos
Copy link
Member

Node warns of a possible memory leak thanks o added event listeners on abortSignal:

(node:52434) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
at EventTarget.[kNewListener] (node:internal/event_target:256:17)
at EventTarget.addEventListener (node:internal/event_target:335:23)
at anySignal (node_modules/any-signal/index.js:27:12)
at node_modules/libp2p/src/dialer/dial-request.js:70:85
at runMicrotasks ()
at processTicksAndRejections (node:internal/process/task_queues:94:5)

This started to happen when we start supporting dnsaddr resolution and js-ipfs started using bootstrap nodes with such addresses. Once these dnsaddr records are resolved, we get a large list of multiaddrs for the given peer, where several addresses are not relevant for JS libp2p dialer as we do not support transports for them (quic).

Despite having several useless addresses, during the dial flow we only checked if we have any transport to dial a peer on the transport-manager layer, which essentially means we are adding unnecessary event listeneres and creating unnecessary promises to be resolved for each of the addresses that will simply be rejected further on the stack.

This PR fixes the above issue by simply filtering out in the beginning of the dial flow the addresses that will not be useful.

Closes #900

@vasco-santos vasco-santos changed the title fix: do not add abort signals to useless addresses fix: do not add abort signals to "useless" addresses Apr 15, 2021
@vasco-santos vasco-santos marked this pull request as ready for review April 16, 2021 07:46
@lidel lidel requested a review from Gozala April 19, 2021 14:50
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Looks good to me

@vasco-santos vasco-santos merged commit 06e8f3d into master Apr 20, 2021
@vasco-santos vasco-santos deleted the fix/do-not-add-abort-signals-to-useless-addrs branch April 20, 2021 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Possible EventTarget Memory Leak Detected
2 participants