Don't create wires until file verification is complete

[roccomuso]

• WebTorrent version: 1.2.3 (webtorrent 0.96.0)
• Node.js version: 4.4.7

This the warning i'm getting:

(node) warning: possible EventEmitter memory leak detected. 11 ready listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Torrent.addListener (events.js:239:17)
    at Torrent.once (events.js:265:8)
    at Request.onChunk [as callback] (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/lib/torrent.js:1488:34)
    at Wire._callback (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/bittorrent-protocol/index.js:604:11)
    at Wire._onPiece (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/bittorrent-protocol/index.js:508:8)
    at Wire._onMessage (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/bittorrent-protocol/index.js:674:19)
    at Wire._write (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/bittorrent-protocol/index.js:592:10)
    at doWrite (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/readable-stream/lib/_stream_writable.js:319:64)
    at writeOrBuffer (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/readable-stream/lib/_stream_writable.js:308:5)
    at Wire.Writable.write (/usr/local/lib/node_modules/webtorrent-cli/node_modules/webtorrent/node_modules/readable-stream/lib/_stream_writable.js:246:11)

[DiegoRBaquero]

So requests are being delayed to the ready event.

If we are already requesting, doesn't that mean we have metadata? @feross

[feross]

We actually start the discovery process right away, even before the 'ready' event fires, which is why we need https://github.com/feross/webtorrent/blob/c5eda255ab89efe8c1d62589a5df4ee7c146397b/lib/torrent.js#L1487-L1488.

Maybe we should wait for the metadata?

[feross]

@roccomuso You can safely ignore this warning. It's not an issue. You can change EventEmitter.defaultMaxListeners to Infinity if you want to make the warning go away.

[DiegoRBaquero]

@feross How come ready event hasn't fired if we already have the information about the pieces, as we are requesting them from wires (thus we have the metadata)?

[feross]

I think that we are doing verification of the existing file data on disk as the last step before the 'ready' event fires.

[xuset]

Hi. I ran into this problem too; same error with the same stack trace. It seems the client is requesting pieces from peers before verification has completed. Which causes many listeners to be attached to the 'ready' event. Would it be better to not request any pieces until after verification? This makes more sense to me because we don't know what pieces we should request until after verification. To test this out, I added this line to _update in torrent.js

Torrent.prototype._update = function () {
  var self = this
  if (self.destroyed) return
  if (!self.ready) return // Add this line
  ......
}

This fixed it, and all the tests still passed. @feross Do you think this is a good fix? Should I submit a PR?

[feross]

@xuset I think this is a good idea. But I think we should solve this by not creating any wires until after verification. Just returning from _update seems a bit hackish.

I'll leave this issue open to track this todo item.

[ryush00]

Any updates?

[pldubouilh]

I ran into the same problem - especially an issue on boxes with high connectivity and low cpu ressources. Based on @xuset fix I had another go:

• Check in _drain to only allow new wires if the torrent is ready or if there's no metadata
• Call _drain when metadata and store are ready to ensure we kick off wire creation
• Check in _update to only allow block requests if the torrent is ready

@feross what do you think ?

Edit: Managed to replicate with the fix, so Seems to also have an impact on #1180 - but need to dig into that one a bit more
Edit: One more check

[KayleePop]

#1350 fixes this I think (even though that wasn't the intention of the PR)

Instead of waiting for the entire store to be verified, requests for a piece are cancelled unless that specific piece is verified. This means that while wires are created before the store is verified, wire.request() won't be called until that piece is verified in the store.

This is essentially the same behavior as now, but the logic is applied for each piece individually instead of the whole store or nothing, and instead of making requests that wait until verification is complete, the request is cancelled and a new one is made once the piece is verified.

Can anyone verify that the problem is solved with these changes?

In the PR, I didn't touch the listener for the ready event in wire.request(), but I think it can be safely removed along with these changes right?

 

EDIT: I think the listener is still necessary for when torrent.critical() is used before file metadata is retrieved (return value from client.add() is used instead of the callback) because critical pieces don't wait for store verification in my PR. I think waiting for the ready event in client.critical() instead would be a good idea though, because that will only create as many listeners as calls to torrent.critical()

 

EDIT 2: Requests for critical pieces are now also cancelled before verification in the PR. It now removes the event listener as well. This is cleaner, and the benefit of downloading critical pieces before verification was probably insignificant, because those pieces are still prioritized for verification.