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

Can FLOW and PROTOCOL be merged / renamed? #368

Open
craigfe opened this issue Dec 3, 2020 · 9 comments
Open

Can FLOW and PROTOCOL be merged / renamed? #368

craigfe opened this issue Dec 3, 2020 · 9 comments

Comments

@craigfe
Copy link
Member

craigfe commented Dec 3, 2020

Conduit currently defines two related module types: FLOW and PROTOCOL: an implementation of FLOW provides an API for conduit endpoints, and PROTOCOL extends this with a connect function using some notion of endpoint.

I find this API a bit confusing, mostly because the nouns "flow" and "protocol" suggest very different things to me – despite their similar types in Conduit – e.g.

  • a "flow" is typically a communication channel, naturally represented as an object;
  • a "protocol" is a set of rules for using a communication channel, naturally represented as a module.

I think this confusion comes across in some places in the code too, where the words "flow" and "protocol" are sometimes seemingly used interchangeably. I toyed a bit with merging these two module types – leaving just a FLOW that supplies connect – in this branch, and it seems quite easy to do.

Are there any use-cases where this is an important distinction? (Perhaps in Cohttp?) If so, are there any other names that might make that distinction clearer?

@samoht
Copy link
Member

samoht commented Dec 3, 2020

That looks like a great simplification :-)

@dinosaure
Copy link
Member

Hmmhmm, the distinction is the big deal of Conduit - separate recv/send/close from connect (as we usually do in MirageOS). Your code should not work (but it seems), I need to introspect your proposal to see what it happens.

@dinosaure
Copy link
Member

Ok, I was disturbed by the diff:

craigfe@da9377d#diff-269aef5e17b2566b4fa76d1e6df55c935e918d11ce30b3b94a9a06bb2775afbfL71-L74

By this update, you give an access to connect to the end-user (with Conduit.impl for example), I'm not sure that is what we want. I need to find a counter case where we should not have an access to this function.

@craigfe
Copy link
Member Author

craigfe commented Dec 3, 2020

Indeed, that diff is fairly important to my proposal 🙂 It seems to me that the end-user should be able to connect if the endpoint type is not abstract to them, and so the safety comes about that way. Thanks for looking into it 🙂

@talex5
Copy link
Contributor

talex5 commented Dec 3, 2020

I generally try to avoid putting constructors in the interface because it usually doesn't add anything (this is a complaint about the original PROTOCOL too). e.g. here we have:

  type endpoint
  val connect : endpoint -> (flow, error) result io

Since it doesn't say anything about endpoint, which is only used once, this type isn't useful to anyone. If you have extra knowledge about what endpoint is, you can also know about the concrete connect function too.

The main things this does are:

  1. Tells us that whatever errors connect can return (e.g. Unknown_address), all the other methods must return too. Which is probably not what we want.
  2. Forces us to pass all information as a single argument, with a single constructor. e.g. if we want to enable tracing here or provide a switch to limit the lifetime, it has to be part of the endpoint (which is always possible, but not always very elegant).

An easy counter example is a single device, such as the null device (of course, you could always define type endpoint = |). I suspect that if you look at the places where this is used, you'll find they could be simplified by calling some concrete function directly.

@craigfe
Copy link
Member Author

craigfe commented Dec 3, 2020

To be clear: my choice to keep PROTOCOL rather than FLOW in the linked test branch was entirely in the interests of getting a quick solution. I agree with your points about the uninformative nature of connect, and perhaps that is the better resolution to this API complexity.

@samoht
Copy link
Member

samoht commented Dec 3, 2020

We need connect and endpoints to register resolvers. Maybe there is a way to just introduce these concepts when we start talking about resolvers?

@talex5
Copy link
Contributor

talex5 commented Dec 3, 2020

@samoht I think that's a different kind of endpoint though. Resolvers use Endpoint.t (a domain name or IP address), whereas PROTOCOL.connect just takes an arbitrary type. e.g. TLS uses Lwt_unix.sockaddr * Tls.Config.client (IP address + port + TLS config) as the endpoint type.

@dinosaure
Copy link
Member

@samoht I think that's a different kind of endpoint though. Resolvers use Endpoint.t (a domain name or IP address), whereas PROTOCOL.connect just takes an arbitrary type. e.g. TLS uses Lwt_unix.sockaddr * Tls.Config.client (IP address + port + TLS config) as the endpoint type.

Yes and we need both 👍. The process of Conduit is to take a concrete type Conduit.Endpoint.t to let the user to describe the dispatch (when he uses Conduit.add). Then, the dispatch delivers a more concrete 'endpoint (such as Lwt_unix.sockaddr but it can be something else) to be able to call the Protocol.connect. Then, the Protocol.connect allocates the ressource (a socket or something else) and we wrap it into the abstract type Conduit.flow.

In others words, the process begins with a concrete type (Endpoint.t) and it finishes with a concrete type (Conduit.flow). However, between these bounds, Conduit uses abstract values (it does not need to know the definition of these values and it does not need to manipulate these values):

(Endpoint.t * Conduit.resolvers) -> 'endpoint -> Protocol.connect 'endpoint -> 'flow -> Conduit.flow

'endpoint and 'flow are truly abstracted and its why we are able to plug a Tls.Config.client (as an 'endpoint) regardless Conduit - but according to Conduit_{lwt,async}_tls.endpoint (which is not the part of the core). This part of Conduit is difficult because it's about all the magic stuff about types.

About the initial question and proposal, I don't have strong opinion to let the connect in the FLOW module type. It seems to work and I did not find (not yet) a counter example where such exposition will be bad. For my perspective, we limit aggressively the usage of Conduit however because, it we are able to have a type-witness of the protocol/flow which keeps type informations such as the 'endpoint and the 'flow AND we are able to re-project the module implementation from this type-witness, why we need to use Conduit.resolve?

I mean, at this stage and considering your update, we can have a simple Hashtbl.t which binds a kind of unique identifier (such as a name) and gives to us the type-witness. But we need to go deeper on this way to see if it's really valuable for our goal, I don't really now 😕 .

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

No branches or pull requests

4 participants