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

Added Endpoint enum to better encode endpoints for different transport types #68

Merged
merged 2 commits into from
Sep 28, 2020
Merged

Conversation

TheButlah
Copy link
Contributor

@TheButlah TheButlah commented Sep 28, 2020

I've added an Endpoint enum, along with a Host and Transport enum. These enums should help us add support for new transport types other than tcp or udp.

Without the enums, I found myself having to check the syntax of the string based endpoint a lot. For example, if its tcp the string should be host:port, but if its ipc it should just be a string address. By consolidating all of these checks into the parse() method of Endpoint, we can avoid scattering these parsing and checking steps everywhere in the code, and we can also let people avoid that overhead by directly constructing the Endpoint.

let str_endpoint: Endpoint = "tcp://example.com:4567".parse()
let enum_endpoint = Endpoint::Tcp("example.com".to_string(), 4567)

Things to discuss:

  1. I have not figured out the proper types to encode the errors in the endpoint module yet - most of them are labeled as ZmqError::Other. What do you recommend the error type be?
  2. I have made a custom trait called TryIntoEndpoint which basically does the same thing as TryInto<Endpoint, Error=ZmqError>. I have the trait because I wanted something similar to a trait alias, but that isn't a thing in rust yet (see Tracking issue for trait aliases rust-lang/rust#41517). Is this the right approach?
  3. I currently accept arguments as a: impl TryIntoEndpoint for the user-facing functions accepting an endpoint. This form of static dispatch will get monomorphized in most cases and lead to code bloat, but lets the user pass either strings or Endpoint types to these functions, which is a little more ergonomic. However, its really not that hard to pass the argument as "tcp://example.com:4567".parse(). It might be worth removing the impl TryIntoEndpoint and just doing a: Endpoint instead. That might also let us delete the TryIntoEndpoint trait entirely, and will lead to faster compile times and smaller binary sizes. Especially if we intend to support WASM as a platform, where binary size really matters, this will be important. What are your thoughts?
  4. The different variants of the Endpoint enum are currently just tuples. For example, I have Endpoint::Tcp(Host, Port). This means that if there are functions that operate exclusively on a particular type of endpoint (lets say TcpEndpoint), we currently have to do a match to make sure that we have the right transport type for any given Endpoint. It might be worth instead making each variant of the Endpoint have a type. For example, we could have both Endpoint and TcpEndpoint, where TcpEndpoint is a tuple struct of (Host, Port), and we would have Endpoint::Tcp(TcpEndpoint). We could also impl From<TcpEndpoint> for Endpoint to allow to easily convert from a TcpEndpoint to an Endpoint. The only downside is that it would introduce a lot of additional types, but I think that its worth it for the increase in type safety

@TheButlah
Copy link
Contributor Author

TheButlah commented Sep 28, 2020

Update: I've removed TryIntoEndpoint and the associated impl TryIntoEndpoint parameters. I think making people do "tcp://example.com:4567".parse()? instead of just the string is not that unreasonable. It makes the function signatures a lot simpler, people don't have to reason about what implements what traits, and it should reduce code bloat.

@Alexei-Kornienko
Copy link
Collaborator

I think it's too early to declare support for all these transport types. IMHO we should start working on at least 1 additional transport and make sure that we can actually support it without big changes in code structure. So imho we should try and implement udp/ipc for example and when we'll be sure that it actually works do all the needed stuff to provide configuration options

Copy link
Collaborator

@Alexei-Kornienko Alexei-Kornienko left a comment

Choose a reason for hiding this comment

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

pls see my comments. Currently I would prefer not to merge this

examples/message_broker.rs Outdated Show resolved Hide resolved
@Alexei-Kornienko
Copy link
Collaborator

For question 1. We already have a Address error variant https://github.com/zeromq/zmq.rs/blob/master/src/error.rs#L8 IMHO all this errors relate to this class

@TheButlah
Copy link
Contributor Author

TheButlah commented Sep 28, 2020

For question 1. We already have a Address error variant https://github.com/zeromq/zmq.rs/blob/master/src/error.rs#L8 IMHO all this errors relate to this class

@Alexei-Kornienko I'll go ahead and make those changes now. In addition, would you be open to me submitting a separate PR to rename ZmqError::Address to ZmqError::Endpoint? That would be more consistent with zmq's distinction between endpoints and addresses. For example, an Ipc endpoint doesn't have an Address

@TheButlah
Copy link
Contributor Author

I think it's too early to declare support for all these transport types. IMHO we should start working on at least 1 additional transport and make sure that we can actually support it without big changes in code structure. So imho we should try and implement udp/ipc for example and when we'll be sure that it actually works do all the needed stuff to provide configuration options

Fair enough. I marked the Endpoint enum as well as the Transport enums as #[non_exhaustive], so I can reduce them down to the minimal set of transports we want to support as a short term goal, and we can add to those enums over time as we grow to support more.

Should I have only the currently supported transport protocal (in this case just tcp) in those enums, or should I add for example Udp too in preparation for adding support for Udp?

@Alexei-Kornienko
Copy link
Collaborator

Fair enough. I marked the Endpoint enum as well as the Transport enums as #[non_exhaustive], so I can reduce them down to the minimal set of transports we want to support as a short term goal, and we can add to those enums over time as we grow to support more.

Yes

Should I have only the currently supported transport protocal (in this case just tcp) in those enums, or should I add for example Udp too in preparation for adding support for Udp?

I would start with TCP only and add other transport types together with respective implementation

@TheButlah
Copy link
Contributor Author

Ok, I've removed all the transport variants except Tcp.

@TheButlah
Copy link
Contributor Author

TheButlah commented Sep 28, 2020

OK, I ended up doing the error handling changes in this PR because it integrated too closely with the Endpoint enum.

I couldn't trivially use the existing ZmqError::Address because it can only be constructed from an AddrParseError, which cannot be manually constructed. Therefore I ended up making an enum specifically for errors when parsing endpoints, and I made the main ZmqError use #[from] EndpointError. I think this gives the desired granularity and specificity, but still allows for easy aggregation of errors into a single ZmqError type.

Alternatively, I could modify the PR to get rid of EndpointError and put the different variants into the ZmqError instead. I personally prefer the current style, but I'd like to know your thoughts.

@Alexei-Kornienko
Copy link
Collaborator

LGTM. Could you please rebase your branch before merging? IMHO you could squash some commits to make history clean and nice

@TheButlah
Copy link
Contributor Author

@Alexei-Kornienko OK, I've rebased and fixed up some of the commits to distill it down to the two main contributions. Merge when ready

@Alexei-Kornienko Alexei-Kornienko merged commit ec6aa58 into zeromq:master Sep 28, 2020
@TheButlah TheButlah deleted the endpoint branch September 28, 2020 19:58
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.

None yet

2 participants