Skip to content

Conversation

@astro
Copy link
Contributor

@astro astro commented Dec 10, 2014

Hi

For these big changes I'm not going to use my commit bit but want you to review. If accepted, I'm going to push future fixes and review incoming PRs. If not, I will probably start maintaining my own fork.

Notes:

  • Greatly reduced function lengths, code smells better now
  • Deduplicated the HTTP/UDP handling, making it more uniform
    • Renamed a few infoHash instances to info_hash fields in the process, easening the handling of protocol input
  • Tried to separate the protocol bits from logic
  • Expose getSwarm()
    • Solves whitelist/filtering of torrent info hashes #44
    • getSwarm() and Swarm objects can now be implemented by users, allowing for custom logic
    • Swarm processes stuff asynchronously, enabling us to store peer data in a database or sync it between tracker instances

Next up

  • We could always use more tests
  • Server-side IPv6 support
  • Tracking completed events for scraping the downloaded count
  • Benchmarking

Consistent formatting of binary identifiers

In the process I dropped the capability of passing hex info_hashes to getSwarm() because that would put an additional requirement to anyone implementing the function themselves. It would be really helpful to have consistent conventions for info_hashes and peer_ids in the Webtorrent scope.

The options are:

  • Buffers: while binary safe, can't be used as dict keys
  • Binary strings: not printable, also I'm afraid of any Unicode mangling
  • Hex strings: printable, safe, my favourite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no reply for udp parse errors? We reply for http parse errors. Can we keep this consistent unless there's a good reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending a full reply for a request with just an invalid byte can become a vector for traffic amplification. Be careful with UDP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@feross
Copy link
Member

feross commented Dec 11, 2014

This is an excellent PR! The shorter functions are much easier to understand and will likely be easier for V8 to optimize since it has a pretty small maximum size for functions which it will consider for optimization. The de-duplication was something I planned to do, but I love the way you accomplished it.

Once you address my comments, I'll merge and do a release. My comments are mostly small style things or questions to clarify why you did things a certain way.

@feross
Copy link
Member

feross commented Dec 11, 2014

As for consistent binary identifiers, I agree that hex is the clearest and most likely to work without encountering difficult-to-understand utf8 issues. I wonder if issues like this one (webtorrent/webtorrent#196) are caused by binary string utf8 issues.

The reason I went with binary strings initially was to avoid converting the binary strings that http clients send in order to do a _getSwarm lookup. But since udp is supported now (and much more common for production trackers) and we get a Buffer in that case, this isn't a good reason anymore. I support switching to hex strings everywhere.

@astro
Copy link
Contributor Author

astro commented Dec 11, 2014

Thank you for the kind words and the useful feedback. I stepped through your suggestions and committed/commented on each of them. I'm looking forward to the merge.

feross added a commit that referenced this pull request Dec 11, 2014
Refactor the server-side code
@feross feross merged commit 20cb182 into master Dec 11, 2014
@feross feross deleted the refactor branch March 24, 2016 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants