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

torrent: cancelBlocks on wire 'close' #253

Closed
wants to merge 1 commit into from

Conversation

@astro
Copy link
Contributor

astro commented Jan 22, 2015

We may need to add even more logic: malicious peers may hold back responses indefinitely. What do you think about request timeouts?

(Albeit not for malicious reasons, I once got no response from certain peers when choosing a wrong request size (> 16 KB)).

@feross

This comment has been minimized.

Copy link
Member

feross commented Jan 22, 2015

@astro When a wire emits finish, all pending request callbacks are called with errors. Code here: https://github.com/feross/bittorrent-protocol/blob/c41d091549dc15261f81d3c22d6d59771a6b9cf1/index.js#L620-L621

This will be handled in gotPiece in torrent.js here: https://github.com/feross/webtorrent/blob/25c6a10e3180b4aeea1c36f8d57ad66a0e599b52/lib/torrent.js#L966 where storage.cancelBlock is called.

Unless I'm mistaken, this PR shouldn't be necessary! Do you agree?

@feross

This comment has been minimized.

Copy link
Member

feross commented Jan 22, 2015

malicious peers may hold back responses indefinitely. What do you think about request timeouts?

Timeouts are already handled by bittorrent-protocol here: https://github.com/feross/bittorrent-protocol/blob/c41d091549dc15261f81d3c22d6d59771a6b9cf1/index.js#L450 as long as we call wire.setTimeout in torrent.js, which we are.

@feross

This comment has been minimized.

Copy link
Member

feross commented Jan 22, 2015

There's even a test verifying that timeouts work as expected here: https://github.com/feross/bittorrent-protocol/blob/master/test/timeout.js

@astro

This comment has been minimized.

Copy link
Contributor Author

astro commented Jan 23, 2015

you're right.

@astro astro closed this Jan 23, 2015
@lock lock bot locked as resolved and limited conversation to collaborators May 13, 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

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