Review thoughts

[carllerche]

I'm working on reviewing the repository. I have not reviewed everything yet. I will track my thoughts as they come here. Actionable takeaways should be tracked in dedicated issues / PRs.

NewTransport

It seems that NewTransport is a Service of transports. Perhaps this should be represented as an "trait alias" similar to MakeService. Naming wise, right now we have MakeService, so perhaps this could be MakeTransport?

pipeline::Client

• Can max_in_flight be removed? This should be possible to implement as a middleware.
• Is the VecDeque of requests needed? Can these be sent directly to the transport without buffering?

pipeline::client::Maker

Should this be named MakeClient?

pipeline::Server

Same as with Client, could some fields / logic be removed in favor of using middleware to implement that logic?

Examples

It would be nice to have a client / server example using a basic line protocol so we can see how the entire stack fits together w/ Tokio and codec / framed.

[jonhoo]

WIP thoughts:

I'm working on reviewing the repository. I have not reviewed everything yet. I will track my thoughts as they come here. Actionable takeaways should be tracked in dedicated issues / PRs.

NewTransport

Yup, I think NewTransport could totally be MakeTransport which is in turn a Service with particular requirements on the types.

pipeline::Client

• Yup. max_in_flight can be removed.
• The VecDeque is needed in case the transport start_send fails.

pipeline::client::Maker

I didn't name it MakeClient (or ClientMaker) because it's already under the client module. Wanted to try to avoid repeating names in the path. Not super wedded to that either way though.

pipeline::Server

We could probably get rid of max_in_flight here too. I find it a little weird to think about max in flight limits as middleware on the server, but should work.

Examples

There are some tests in tests/ that have examples using async-bincode, but larger examples with a full codec might also be helpful. I haven't used codec/framed before myself, but should fit in nicely.

[jonhoo]

From some discussion: probably fine to replace the VecDequeue for requests with a single "slot" to buffer a failed start_send, and then do the send of that in poll_ready.