Fixes #129

Summary of changes:

• add trait streaming::multiplex::RequestIdSource for generating RequestIds
  • trait can either take the id out of the message or generate it, like Counter
  • refactored old incrementing u64 counter as streaming::multiplex::Counter
  • created in streaming::multiplex::ClientProto, typename is RequestIds
• add trait streaming::multiplex::RequestId which is Hash + Eq + Copy + Debug
• all traits and structs that depended on RequestId have now received associated type RequestId
• tests now use u64 explicitly as Frame<u64, ...>
• simple::multiplex::* continues to use u64 and streaming::multiplex::Counter

Naming

Originally I was thinking about naming the new requirements for RequestId as RId and to keep the old for backwards compatibility. In the latest commit RId has been removed in favor of recycling the old RequestId name.

The associated type for RequestIdSouce in streaming::multiplex::client::ClientProto is RequestIds. This could be more descriptive, open for suggestions.

simple::multiplex::* using u64 and Counter

I am not sure if this makes much sense, and if there are no objections I'll try to make these generic as well. This would keep the simple side usable even if streaming is not needed.

Edited to reflect latest changes (mostly naming).

• [x] Destiny of old RequestId and the new RequestId
• [x] Name of streaming::multiplex::client::ClientProto::RequestIds (RequestIdSource)
• [x] Make simple::multiplex::* generic as well

Some protocols use arbitrary or otherwise non-numeric strings as command IDs. An example could be IMAP, which allows anything alphanumeric (https://tools.ietf.org/html/rfc3501#section-2.2.1), or NetConf, which uses an arbitrary string (https://tools.ietf.org/html/rfc6241#section-4.1).

Would it make sense to allow the user of the multiplexed protocol to specify the type for RequestId?

Hey @carllerche! I'm super excited to try out tokio later; it looks really well done.

Just a thought as I was reading the initial announcement - I noticed the associated type names on Service are all shortened. Do you feel strongly about keeping the names like that? Naturally I can't speak for anyone other than myself, but I find readability goes down when reading something like Self::Fut vs Self::Future. The latter seems more descriptive and potentially more friendly.

I propose using the full names for the associated types. Also, the shed should be blue.

pub trait Service: Send + 'static {

    /// Requests handled by the service.
    type Req: Send + 'static;

    /// Responses given by the service.
    type Resp: Send + 'static;

    /// Errors produced by the service.
    type Error: Send + 'static;

    /// The future response value.
    type Fut: Future<Item = Self::Resp, Error = Self::Error>;

    /// Process the request and return the response asynchronously.
    fn call(&self, req: Self::Req) -> Self::Fut;
}

The services accepted by TcpServer::serve wire up the service's request/response types the underlying request/response types of the BindServer implementation. These types tend to be what we want for the simple case (non streaming), but for the streaming case they have the tokio_proto::streaming::Message type wired in.

The Message type typically isn't what libraries want as the request/response types for their services, so this PR generalizes to use From and Into to ensure that custom types can still be used so long as they can be converted.

I think this sould be backwards compatible as well (not causing inference regressions), hopefully at least.

Right now the with_handle method of TcpServer allows access to a handle of the core created by serve. It would be useful to allow the core instance to be passed in (or spawn the server using a provided handle).

Without such an API anything that needs access to the handle (e.g. other initialization that requires a handle) has to go through the with_handle call of TcpServer.

Whether a spawn vs run API (or both) is added is debatable.

Commented here: https://github.com/tokio-rs/tokio-proto/pull/62#discussion_r82087908

I personally interpret in/out with one of two interpretations:

• Read out a request, write in a response.
• Read in a request, write out a response.

Unfortunately these mean opposite things :(. Looks like this is standard terminology in finagle though? (cc https://github.com/tokio-rs/tokio-proto/pull/62#discussion_r82124234). Maybe I should just shift how I view these?

(may also be worth exploring alternative names)

I don't know if this is an expected behavior, but it seems that a TcpClient somehow leaks its internal connection (a TcpStream instance?) into the tokio_core::reactor::Core it's running on.

I created an example test case that has two tests. One test forces the Core instance to drop before the test asserts that the connection is closed (somewhat indirectly, by checking if a dummy server thread has finished). The other lets the Core instance live beyond the assertion, so it's only dropped in the end of the test. The first test succeeds, while the second fails.

It would be nice if tokio-proto supported non-client server applications.

Currently a user can decide between streaming vs simple and pipelined vs multiplexed. By the same token, they could also decide between client-server and P2P. It might also be desirable to support something in between client-server and P2P, where the server can push messages which the client does not have to respond to.

To keep duplicated code to a minimum, my thinking was to use the same strategy as streaming vs simple, where the two share an implementation, but one (simple) only exposes some of the functionality. In this case, the base implementation would be P2P, and the non-P2P client and server would simply not expose the P2P functionality.

Is this in scope for the project? And if so, does the above strategy seem reasonable?

This commit adds a new module to tokio-proto, easy, which like tokio_core::easy is intended to help you get off the ground running quickly for any particular protocol.

This module mirrors the API of the pipeline/multiplex modules with a server/client implementation that's built on top of the actual server/client in the top-level modules. The types and generics, however, are greatly reduced to help work with them ergonomically.

For some protocols like msgpack-rpc or json-rpc, some requests have IDs and expect a responses with the same IDs, but they also have notifications, which don't have IDs and don't expect a response. I'm not sure how to handle this.

When decoding messages without an ID, I can just create a fake one (u64::max_value() for example). The real problem is when implementing the service. For a client it will look like this:

impl Service for Client {
    type Request = Message;
    type Response = Message;
    type Error = io::Error;
    type Future = Box<Future<Item = Message, Error = io::Error>>;

    fn call(&self, req: Self::Request) -> Self::Future {
        // my future will never return for messages that don't expect a response
    }
}

So is tokio-proto what I really need for such protocols? If not, what should I look at implementing?

Here's a gist for a general concept of the problem: https://gist.github.com/daiheitan/35a089e2065f96a9c985c27e57f286dd

Hitting: https://github.com/tokio-rs/tokio-proto/blob/overhaul/src/pipeline/pipeline.rs#L270

Is this library still being maintained? Please upgrade the version of smallvec and release the latest tokio-proto. The old version of this library was checked for security holes.

cargo audit
    Fetching advisories `https://raw.githubusercontent.com/RustSec/advisory-db/master/Advisories.toml`
    Scanning 340 crates for vulnerabilities (8 advisories in database)
     Warning Vulnerable crates found!

ID: RUSTSEC-2018-0003
Crate: smallvec
Version: 0.2.1
Date: 2018-07-19
URL: https://github.com/servo/rust-smallvec/issues/96
Title: Possible double free during unwinding in SmallVec::insert_many
Solution: upgrade to: >= 0.6.3

ID: RUSTSEC-2018-0003
Crate: smallvec
Version: 0.6.1
Date: 2018-07-19
URL: https://github.com/servo/rust-smallvec/issues/96
Title: Possible double free during unwinding in SmallVec::insert_many
Solution: upgrade to: >= 0.6.3

error: 2 vulnerabilities found!

relate ：https://github.com/tokio-rs/tokio-proto/pull/198

Writing a library that depends on tokio cannot use tokio-proto::TcpClient of course:

   = note: expected type `&tokio_core::reactor::Handle`
              found type `&tokio::reactor::Handle`

Updating the dependency would solve that problem but it's a breaking change, isn't it?

As a library author I'd love to get some more clarity about the future of tokio-proto. Within the tokio-reform I can read the following:

So, in general: the future direction for tokio-proto is unclear.

This is not a good foundation for tokio-library developers, is it?

We need to be driven by strong, concrete use-cases.

What exactly do you mean by "strong"?

If you have one of these, we'd love to hear about it!

It might not the most popular use case but nevertheless it is one of many: tokio-modbus. I recently added a server implementation based on tokio_proto::TcpServer. It kind of works but I don't see that this is the best solution. E.g. there is no way to configure the TcpStream (timeouts etc.).

I love to spend a lot of time to improve the tokio project and the rust ecosystem in general. And to do so clarity would help a lot :) I really would like to get some feedback on how I should behave.

Fixed BoxFuture-related warnings

Dependency updates

• futures 0.1.14 -> 0.1.17
• log 0.3.6 -> 0.3.8
• smallvec 0.4.0 -> 0.6.0
• tokio-core 0.1.8 -> 0.1.12