Limit peers in tracker server with LRU based cache #160
Conversation
| while ((peerId = ite()) && peers.length < numwant) { | ||
| var peer = this.peers[peerId] | ||
| // Don't mark the peer as most recently used on announce | ||
| var peer = this.peers.peek(peerId) |
There was a problem hiding this comment.
There is a problem here, on one hand, if I use LRU.get, the peer will be marked as used and the max condition can evict a more recent peer.
On the other hand, if I use LRU.peek, we never access old peers and the maxAge eviction will not happen.
I think I will clean old peers manually as they are picked in announce...
yciabaud
changed the title
lib/server/swarm.js
Outdated
| while ((peerId = ite()) && peers.length < numwant) { | ||
| var peer = this.peers[peerId] | ||
| // Check manually if the peer is active | ||
| if (peers.maxAge && (Date.now() - peers.cache[peerId].modified) > peers.maxAge) { |
There was a problem hiding this comment.
Maybe this deserves a PR to LRU too, I am trying to access an element without bringing it up in the chain but with evicting it if its too old.
|
@yciabaud Aside from the comments I left LGTM. We need to evaluate the case I stated here: #4 (comment) It's a problem in the long run. Each swarm has an LRU cache which in some case might never remove peers, consuming RAM. OpenWebTorrent trackers, who's been running without restarting for the longest shows us this, it's full of peers who left ungracefully: https://tracker.openwebtorrent.com/stats The default limit of 10K peers for a single torrent is huge IMHO. On another topic, what do you guys think about adding the version number in the stats page & json? @feross @jakefb |
|
Well, the case you're pointing out is important but lets stay at the swarm level for this PR and then try to work on removing old torrents with its swarm in another one. Yeah, 10K is huge but it was infinite before. What would you set? 1K? And to answer your last question, I do agree that some more infos in the stats would not hurt, I am thinking of name, version, uptime... |
|
1K peers per torrent should do. This PR should update LRU dep to 3.0.0 and use the new .keys method and peek maxAge check. |
|
Sure will update this today. If peek checks maxAge, calling stats will prune old peers on unused torrents on my peer stat PR :-D |
|
Oh, yes! I hadn't thought about it, we will peek them now on stats and that prunes them. All problems solved now! (Well at least in small scale (1k dead peers on a tracker) don't accumulate in months as far as I can see from owt's tracker) I remember when 400 peers would kill my server's memory with the memleaks there were. Now 900k peers go for about 380MB of Ram. Diego R.
|
|
We should not rely on this but this is a quick way to clean the memory. ;) |
lib/server/swarm.js
Outdated
| this.peers = {} | ||
| this.peers = new LRU({ | ||
| max: server.peersCacheLength || 1000, | ||
| maxAge: server.peersCacheTtl || 900 // 900s = 15 minutes |
There was a problem hiding this comment.
This should be in milliseconds, according to the docs for lru
|
LGTM now. @feross ? |
|
@feross, I would rather have a last feedback from you since it is core related before merging this. |
|
Finally got a chance to look at this. LGTM! |
3 participants
As discussed in #4, we have to limit the peers stored in the tracker server. This PR uses a configurable
lrucache with 2 rules:Notice that peers last used date are refreshed on each request to the server.