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

RFC: Handshaker Redesign #48

Closed
GGist opened this issue Apr 7, 2016 · 4 comments
Closed

RFC: Handshaker Redesign #48

GGist opened this issue Apr 7, 2016 · 4 comments

Comments

@GGist
Copy link
Owner

GGist commented Apr 7, 2016

Problem

The bip ecosystem is meant to contain a modular set of crates that expose functionality and services to clients wishing to leverage BitTorrent infrastructure in their applications. We want to be able to provide flexibility for a client so that they can painlessly integrate either a single crate or many crates from our ecosystem into their application. In the case of a single crate, that crate should provide a usable interface to clients; in the case of many crates, those crates should provide a unified interface to clients.

Because of this, we cannot afford to export a per crate asynchronous or synchronous interface for clients to use as that would force a specific architecture on our clients for the purposes of tieing our API into their application.

Goal

Provide a generic interface that clients can use to accept callbacks from every peer discovery service that our ecosystem offers. This callback interface should accept at the bare minimum:

  • A callback for connecting to a peer found via the peer discovery service
  • A callback for metadata that was found during peer discovery

Proposal

My proposal is to modify the current Handshaker to adhere to the following interface:

/// Handshaker for peer discovery services which may or may not contain request metadata.
trait Handshaker: Send {
    /// Type that the metadata will be passed back to the client as.
    type Envelope;

    /// PeerId exposed to peer discovery services.
    fn id(&self) -> PeerId;

    /// Port exposed to peer discovery services.
    fn port(&self) -> u16;

    /// Connect to the given address with the InfoHash and expecting the PeerId.
    fn connect(&mut self, expected: Option<PeerId>, hash: InfoHash, addr: SocketAddr);

    /// Sets a new filter to filter requests based on an InfoHash and SocketAddr.
    fn filter(&mut self, filter: Box<Fn(InfoHash, SocketAddr) -> bool + Send>);

    /// Send the given Metadata back to the client.
    fn metadata(&mut self, data: Self::Envelope);
}

Handshaker implementations would typically accept some channel that an Envelope can be sent over and make sure that the result they yield in response to a connect can be convertible to an Envelope type. Similarly, when a client goes to use the Handshaker in a peer discovery service, the metadata returned from that service must also be convertible to an Envelope.

As an example, lets see how we would integrate a BTHandshaker, as well as one or more peer discovery services in with a mio event loop. A concrete BTHandshaker would accept a mio channel that can send Envelope types. The BTHandshaker impl would assert that the user has provided a From implementation for creating an Envelope from a TcpStream. The client then goes over to a TrackerClient and tries to create one using our BTHandshaker as the generic Handshaker. The TrackerClient impl would assert that the user has provided a From implementation for creating an Envelope from SomeMetadata. Similarly, for every peer discovery service the client uses, this would be enforced for the service specific metadata.

For services which receive no metadata, a generic Handshaker would be accepted and no constraint would be put on the contained Envelope.

With this example, we can see how a mio event loop is now integrated with a number of peer discovery services and can accept both metadata as well as connections over a single channel.

Positives

  • Zero cost abstractions for the user
  • User pays only for the services they use
    • Only needs From impls for the initial BTHandshaker as well as the services it uses
  • Unified interface for any of our existing or future peer discovery services to hook in to
  • Specific architecture not enforced on the client
    • Handshaker makes _little_ assumptions about the underlying transport
    • Can use any communication primitive when implementing a Handshaker

Negatives

  • Since Handshaker is manipulated in a peer discovery service's own thread, synchronous programming requires a bit more effort
  • Increased complexity
  • Increased up front cost for the user due to:
    • Conversion impls
    • Indirectly related to this RFC: Accepting a generic channel in, for example, a BTHandshaker requires user's to wrap types such as mio's Sender
@GGist
Copy link
Owner Author

GGist commented Apr 7, 2016

Currently have some example code illustrating the above RFC, will upload soon.

@GGist
Copy link
Owner Author

GGist commented Apr 7, 2016

Minimal example is up: https://github.com/GGist/bip-rs/tree/rfc_handshaker_example

Ping @astro

@GGist
Copy link
Owner Author

GGist commented Apr 8, 2016

As a second thought, I think leaving off filter from the interface is best as I don't see that function being used generically. We could simply offer that functionality in concrete Handshakers.

@GGist
Copy link
Owner Author

GGist commented Apr 14, 2016

Final Comment Period, will close as accepted in two days.

May change the naming of some of the items in the trait. Particularly, a more descriptive and specific name than Envelope since that Envelope could be metadata specific, it doesnt have to be the same type that connections are sent back to the client as.

A concrete Handshaker could decide to send connections down one channel and metadata down another for example. In that case, Envelope would not have a connections enum variant (like in our example). They could even choose to set Envelope as () and throwaway whatever is passed to it (we could provide From implementations for () for all metadata to support throwing away metadata like this without user's having to throw it away themselves).

Also, keep in mind that this metadata is what we will eventually use to send back information from DHT get and put commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant