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

should remove part of memory leaks #382

Closed
wants to merge 1 commit into from

Conversation

@arestov
Copy link

arestov commented Jul 19, 2015

memory leaks through closures of callback in event listeners in foreight objects.

destroy should unsubscribe current object from any events in foreight objects

@arestov arestov force-pushed the arestov:fix-callbacks-leak branch 3 times, most recently from 33fd010 to c1cecc2 Jul 19, 2015
@arestov arestov force-pushed the arestov:fix-callbacks-leak branch 2 times, most recently from 5b07d88 to c2e6dc7 Jul 20, 2015
@arestov arestov mentioned this pull request Mar 30, 2016
@arestov arestov force-pushed the arestov:fix-callbacks-leak branch 2 times, most recently from 632436f to 44e486b Mar 31, 2016
@arestov

This comment has been minimized.

Copy link
Author

arestov commented Mar 31, 2016

also self.swarm.listen( here can be problem

@rom1504

This comment has been minimized.

Copy link
Member

rom1504 commented Mar 31, 2016

I think it would be interesting to have some way to measure how much the memory leaks decreased.

@arestov

This comment has been minimized.

Copy link
Author

arestov commented Mar 31, 2016

You can use this trick.
#712 (comment)

Add this in every file and Class (instances of which could be destroyed) in every webtorrent module.
That run webtorrent for few hours. Or just add 10 torrent and then remove them all.
Take snapshot and you will see how many objects should be GCed by counting of MemoryLeak (filter by name)

var MemoryLeak = function(){}


Item.die = function() {
  // ...
  this.leak = new MemoryLeak()
  // ...
}

I guess it can be added in master branch with process.NODE_ENV == 'DEBUG'

var isDebugging = process.NODE_ENV == 'DEBUG';

//...

Item.die = function() {
  // ...
  if (isDebugging) {
    this.leak = new MemoryLeak()
  }
  // ...
}

(If your code has no leaks than GC will remove whole item, but if not — you will see MemoryLeak in memory profiler (snapshot))

@arestov

This comment has been minimized.

Copy link
Author

arestov commented Mar 31, 2016

Well, I've put MemoryLeak in some of files
leak_detect

Started webtorrent-desktop (whithout code in commit) download and got some leaks

leaks1

Click start/stop and got more leaks

leaks2

So looks like this commit 44e486b is not enough

I guess that this code is also problem:
https://github.com/feross/torrent-discovery/blob/master/index.js#L76
https://github.com/feross/torrent-discovery/blob/master/index.js#L58

…teners in foreight objects
@arestov arestov force-pushed the arestov:fix-callbacks-leak branch from 44e486b to 978ed89 Mar 31, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.