Fix stats and leaked websockets #198

feross · 2017-02-03T01:43:47Z

Possibly fixes: #196

Close websockets when peers are evicted from LRU cache, otherwise it's
possible for a peer object to be evicted from the LRU cache without the
socket being cleaned up. That will leak memory until the websocket is
closed by the remote client. It also messes up the stats.

DiegoRBaquero · 2017-02-03T02:54:49Z

lib/server/swarm.js

+    if (peer.socket) {
+      setTimeout(function () {
+        try {
+          peer.socket.close()


Shouldn't we close the socket (so it sends stopped) and then create the timeout that nullifies it?

Once the socket is closed by the server, there is no way to send messages to the client. So I think it's fine to set peer.socket to null immediately after we close the socket.

The timeout is only needed for the case where the client sends a 'stopped' message. When the server gets the message it calls lru.remove() which emits 'evict' immediately. We need the timeout so that the server can send a response with socket.send().

LRU .remove method does not emit evict. See chriso/lru/index.js#L30

But if the peer is getting evicted because of LRU limit, it will be evicted and stopped message won't ever be received, because we are not receiving anything. We should perform the stuff done in the event of stoppped message when the eviction happens. No need for timeout. I would however close and nullify on nextTick.

LRU .remove method does not emit evict.

Good point. I needed the setTimeout to get the tests to pass, but it looks like I misunderstood what remove() was doing. I'll look again.

We should perform the stuff done in the event of stopped message when the eviction happens.

That's what happens when we call socket.close(). The onWebSocketClose() function will run, and that does a simulated 'stopped' event.

Ahhh, right. Then try closing but nullifying on nextTick.

I don't think we need to nullify on nextTick. Doing it in the same tick should work fine, right?

DiegoRBaquero · 2017-02-03T04:24:13Z

lib/server/swarm.js

  this.peers = new LRU({
    max: server.peersCacheLength || 1000,
-    maxAge: server.peersCacheTtl || 900000 // 900 000ms = 15 minutes
+    maxAge: server.peersCacheTtl || 20 * 60 * 1000 // 20 minutes


Why not keep it low? I mean, clients are supposedly updating every 2 minutes.

This value is used for the udp and http trackers too.

I took a look at the source of opentracker (the most popular tracker software) and noticed they use 30 minutes on the server, but they tell clients to announce every 15 minutes by default.

We're currently telling clients to announce every 15 minutes and purging them at the same time. Which means if they're only a few seconds late, they'll already be removed, and they'll need to be re-added to the LRU. Better to give them some leeway.

For websocket connections, I think this value probably matters less. If the client closes the websocket, they are removed immediately (now that we added the 'evict' handling). So, the only possible downside of a large value for a websocket tracker is if someone opens a websocket but doesn't send any data on it for a long time. The server will keep the connection open for 20 minutes now. But no well-behaved clients should do that, right?

The LRU is shared for all tracker types, so we need to keep it > 15 min.

DiegoRBaquero · 2017-02-03T04:34:34Z

What if you get evicted from one swarm but are active in others? I think we shouldn't close the socket in that case.

feross · 2017-02-03T04:35:21Z

What if you get evicted from one swarm but are active in others? I think we shouldn't close the socket in that case.

Another good point 👍 I'll fix that up.

feross · 2017-02-03T04:51:47Z

Fixed up.

DiegoRBaquero · 2017-02-03T16:22:12Z

server.js

+  debug('websocket close %s', socket.peerId)
+
+  if (socket.peerId) {
+    socket.infoHashes.slice(0).forEach(function (infoHash) {


.slice(0) does nothing

We need to clone the array before looping over it, otherwise it behaves incorrectly because we're mutating it inside the loop here: https://github.com/feross/bittorrent-tracker/pull/198/files/ebe3c218feab4f5a6774d5092531c6b2e55fe608#diff-831f46884145ede920506b29d5681ab2R104

You can read about this behavior of arrays here: https://stackoverflow.com/questions/21811630/splicing-a-javascript-array-from-within-the-callback-passed-to-foreach

DiegoRBaquero · 2017-02-03T17:57:09Z

lib/server/swarm.js

+  // so the stats get updated correctly.
+  this.peers.on('evict', function (data) {
+    var peer = data.value
+    this.announce({


This won't work, peer is already evicted. _onAnnounce gets it and passes it to _onAnnounceStopped as null/undefined.

We should only perform what's inside _onAnnounceStopped:

if (peer.complete) this.complete -= 1 else this.incomplete -= 1

You are correct. Also, I was using this instead of self here.

Possibly fixes: #196

…cache

Possibly fixes: #196 Close websockets when peers are evicted from LRU cache, otherwise it's possible for a peer object to be evicted from the LRU cache without the socket being cleaned up. That will leak memory until the websocket is closed by the remote client. It also messes up the stats.

before this change, it was getting initialized immediately, since it was outside a tape test block

From comment: #198 (comment) 95

Caught this issue because of the new eviction tests. Essentially, this change moves the socketPool into the client instance instead of a reused variable at the module level. When a client sends stop (or is evicted) the server will close the websocket connection if that client is not in any other swarms (based on peerId). However, if we are using a single socket for multiple clients (as was the case before this commit), then other clients will have their sockets unintentionally closed by the server.

feross · 2017-02-08T21:22:33Z

I just added a test to ensure that eviction works correctly. And in the process of adding that test, fixed another bug related to how the socketPool works.

I think this is finally ready to merge.

feross · 2017-02-09T22:16:25Z

You're right about nothing being done with the params, but I wanted to be consistent in case they are used in that function later. Thanks again for the thorough review!

feross mentioned this pull request Feb 3, 2017

incomplete + complete != # of peers #196

Closed

DiegoRBaquero reviewed Feb 3, 2017

View reviewed changes

feross added 15 commits February 8, 2017 12:27

Fixes for PR #185

27ae148

Fix bug where left = 0 would be converted to Infinity

c2c8e36

Possibly fixes: #196

If peer is already complete, it should still be refreshed in the LRU …

ed3da2f

…cache

treat unexpected 'completed' events as 'updated'

62dd0f7

style: re-order methods

6351f2b

swarm maxAge: increase to 20 min

8f33b95

fix tests: wait for socket to send final responses

1b22b53

remove unneeded timeout

29d4564

Only close websocket when it's not participating in any more swarms

d534582

test: ensure electron-webrtc is started only for test where it's needed

e731106

before this change, it was getting initialized immediately, since it was outside a tape test block

test: server: check that all clients, server are destroyed

4422067

add failing tests for desired evicition behavior

cd4a976

Address @DiegoRBaquero's feedback

806ce1d

From comment: #198 (comment) 95

feross force-pushed the fix-196 branch from ebe3c21 to 3f3db7d Compare February 8, 2017 21:22

DiegoRBaquero approved these changes Feb 8, 2017

View reviewed changes

feross merged commit a469740 into master Feb 9, 2017

feross deleted the fix-196 branch February 9, 2017 22:16

DiegoRBaquero mentioned this pull request Feb 18, 2017

TypeErrors: infoHashes is null #205

Closed

Uh oh!

Fix stats and leaked websockets #198

Fix stats and leaked websockets #198

Uh oh!

Conversation

feross commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoRBaquero commented Feb 3, 2017

Uh oh!

feross commented Feb 3, 2017

Uh oh!

feross commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feross commented Feb 8, 2017

Uh oh!

feross commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants