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

fix(wallet-lib): eventemitter memory leak #56

Merged
merged 5 commits into from
Nov 26, 2021

Conversation

markin-io
Copy link
Contributor

@markin-io markin-io commented Nov 24, 2021

Issue being fixed or feature implemented

The wallet was hooking on lots of redundant event listeners during the processing raw transactions from the stream.

What was done?

Remove event listeners right after they'd became not needed

How Has This Been Tested?

  • Manually broadcast 10 transactions one by one to ensure that the warning does not appear anymore
  • Ensure that the warning is not present in the logs of the test suites

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@markin-io markin-io changed the title fix(wallet): MaxListenersExceededWarning: Possible EventEmitter memory leak detected. fix(wallet): eventemitter memory leak Nov 24, 2021
@markin-io markin-io changed the title fix(wallet): eventemitter memory leak fix: eventemitter memory leak Nov 24, 2021
@markin-io markin-io added this to the v0.22.0 milestone Nov 24, 2021
@markin-io markin-io changed the title fix: eventemitter memory leak fix: wallet eventemitter memory leak Nov 24, 2021
@markin-io markin-io changed the title fix: wallet eventemitter memory leak fix(wallet): eventemitter memory leak Nov 24, 2021
@markin-io markin-io changed the title fix(wallet): eventemitter memory leak fix (wallet): eventemitter memory leak Nov 24, 2021
@markin-io markin-io changed the title fix (wallet): eventemitter memory leak fix: wallet eventemitter memory leak Nov 24, 2021
@markin-io markin-io changed the base branch from v0.22-dev to master November 25, 2021 10:39
@markin-io markin-io changed the title fix: wallet eventemitter memory leak fix(wallet-lib): wallet eventemitter memory leak Nov 25, 2021
@markin-io markin-io changed the title fix(wallet-lib): wallet eventemitter memory leak fix(wallet-lib): eventemitter memory leak Nov 25, 2021
@shumkov
Copy link
Member

shumkov commented Nov 25, 2021

@markin-io
Copy link
Contributor Author

markin-io commented Nov 25, 2021

@shumkov this one is attached to the DAPIClientTransport. I guess it needs to be there.
The one I worked with comes from the class Account extends EventEmitter.

@shumkov
Copy link
Member

shumkov commented Nov 25, 2021

@markin-io We've added that one actually to solve the same thing. Maybe mistakenly? Should we remove it?

Also, you solution doesn't create new listeners for the same transactions. But you'll get warning if you will have more than 11 transaction to get in the block, right?

@markin-io
Copy link
Contributor Author

@shumkov after the removal of the setMaxListeners(0) I couldn't see any difference, so there is a chance that it could've been added by mistake.

As of the warning for more than 11 transactions, this is true. I will think of some kind of a queue to solve this.

Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

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

Good catch! 👍

@markin-io markin-io merged commit 0d0167b into master Nov 26, 2021
@markin-io markin-io deleted the fix/max-listeners-warning branch November 26, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants