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

Memory leaks [META] #712

Closed
arestov opened this issue Mar 30, 2016 · 12 comments
Closed

Memory leaks [META] #712

arestov opened this issue Mar 30, 2016 · 12 comments

Comments

@arestov
Copy link

@arestov arestov commented Mar 30, 2016

Webtorrent and its modules has a lot of code similar to this simple example, which is actually a good example of memory leak:

var Item = function(node) {
  var self = this

  this.node = node

  window.addEventListener('resize', function resizeHandler() {
    if (!self.node) {return}
    self.node.textContent = 'window width: ' + window.width
  })
}

Item.die = function() {
  this.node = null
}

Using:

var item = new Item(document.createElement('div'))
item.die()

resizeHandler still in window resize events, it still has referrence to variable self,
yes we made node free, but item stuck in memory;

To fix it we should not have closures in event listeners after die:

var Item = function(node) {
  var self = this

  this.node = node

  this.resizeHandler = resizeHandler
  window.addEventListener('resize', resizeHandler)
  function resizeHandler() {
    if (!self.node) {return}
    self.node.textContent = 'window width: ' + window.width
  }

}

Item.die = function() {
  window.removeEventListener('resize', this.resizeHandler)
  this.node = null
}

(you can imaging that setTimeout(fn, 30000) and even process.nextTick/setImmidiate will be problem for GC too)

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

#551:
https://github.com/feross/webtorrent/blob/master/lib/peer.js contains events subscribings, but not unsubscribings
(and lack of memory leads to CPU problems)

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

If you do not unsubscribe peers that you remember all of them #248

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

I believe here is same problem
#317
webtorrent/webtorrent-desktop#252

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

Can not to not mention here
#207
#282
#382

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

@arestov

This comment has been minimized.

Copy link
Author

@arestov arestov commented Mar 30, 2016

Here is one more trick

var MemoryLeak = function(){}

var Item = function(node) {
  var self = this

  this.node = node

  window.addEventListener('resize', function resizeHandler() {
    if (!self.node) {return}
    self.node.textContent = 'window width: ' + window.width
  })
}

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

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

@arestov arestov changed the title Memory leak [META] Memory leaks [META] Mar 30, 2016
@grunjol

This comment has been minimized.

Copy link
Contributor

@grunjol grunjol commented Mar 31, 2016

So a simple way to make the code clean can be encapsulate event listeners calls with some kind of listener registry and call an 'unregister' function on destroy?

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Apr 1, 2016

@arestov Thanks for trying to get to the bottom of this.

I just cleaned up the leak in torrent-discovery and I'm looking at the other ones now.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Apr 21, 2016

I think I've finally fixed all the leaks. Huge PR coming soon.

@rien333

This comment has been minimized.

Copy link

@rien333 rien333 commented Jan 10, 2018

Although leaking memory seems to be fixed, an examination between the memory behavior of my macbook pro and rpi causes me a lot of confusion. It is my understanding that webtorrent-cli tries to download a file to RAM (and sometimes to the HD as well). At least this is what it seems to be doing when I launch it as webtorrent-cli --mpv on my rpi. Namely, htop shows that my RAM progressively fills up, and then brings the system to a halt when it has to use the swap file. This problem does not occur when I launch webtorrent without the --mpv flag, and then run mpv separately from webtorrent on the downloaded file.

On my MPB however, launching with --mpv does not seem to fill up my ram lots , and most definitely does not bring the machine to an halt, independent of the size of my torrent. How can this difference exist? Am I out of luck using webtorrent-cli --mpv on my rpi because it doesn't have sufficient RAM? Is garbage collection/memory handling just better/more stable under macOS?

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented May 10, 2018

@rien333 webtorrent-cli stores files in /tmp/webtorrent so it should not be filling up RAM. Do you have your Raspberry Pi set up to store files in /tmp in RAM for read-write performance? I think this is called a "RAM disk".

@rien333

This comment has been minimized.

Copy link

@rien333 rien333 commented May 10, 2018

I am fairly sure this was just a bug in mpv for the Pi at one point. (the devs have been kinda neglicant of it so i think it could have been fairly specific). I think I eventually found out that the RAM filled up pretty fast while streaming basically anything. Anyway, It's fixed now.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.