Skip to content
This repository has been archived by the owner on Sep 13, 2018. It is now read-only.

Allow multiplexed protocols to use different ID types #129

Closed
vorner opened this issue Jan 18, 2017 · 11 comments
Closed

Allow multiplexed protocols to use different ID types #129

vorner opened this issue Jan 18, 2017 · 11 comments

Comments

@vorner
Copy link

vorner commented Jan 18, 2017

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?

@carllerche
Copy link
Member

Definitely possible... It would be worth taking a look to see how much of a headache it would be to make RequestId generic. It would most likely need to have some sort of trait bound to enable the client to generate new request IDs.

@vorner
Copy link
Author

vorner commented Jan 19, 2017

Thinking about generating the IDs, I believe an iterator could be quite close to that. It would be 0.. in case of numbers and something else in the other cases.

OK, I'm going to add this to the (already not so short) list of things I want to try to write O:-).

@carllerche
Copy link
Member

An iterator would probably be best. I guess there should be a RequestId trait w/ an associated iterator.

I'm hoping that this won't complicate the generics too much..

@koivunej
Copy link

I've created a PR for making RequestId a Clone + Hash + Eq + Debug + 'static. Instead of using iterators I've added a new trait named RequestIdSource<Id, T>: 'static which has

fn next(&mut self, msg: &T) -> Id;

I decided not to go with the iterator but instead added ability to inspect the message, as I have a use case where it might be beneficial to allow user of my library to select the Uuid used.

Please have a look at the PR when you have a chance.

@djc
Copy link
Contributor

djc commented Apr 27, 2017

One other concern (I've also been looking at IMAP) is that I think some messages in IMAP are server-initiated; they would effectively not have a RequestId. I'm thinking this could be handled by using Option<RequestId>, but not sure that would make sense? Question would be who/what will handle the server-initiated "responses".

@koivunej
Copy link

I think some messages in IMAP are server-initiated; they would effectively not have a RequestId. I'm thinking this could be handled by using Option, but not sure that would make sense?

I am actually facing a similar issue at the moment as this. In my case the messages go:

C: request_id: 1, payload: "subscribe to Foo"
S: request_id: 1, payload: "subscription started"
...
S: request_id: 1, payload: "notification on Foo"
... until either
C: request_id: 1, payload: "stop subscription"
S: request_id: 1, payload: "stopped because you unsubscribed"
...or...
S: request_id: 1, payload: "stopped because $reason"

If I understood the IMAP IDLE and NOTIFY RFC's correctly it'd seem IDLE serves as a flag to receive any change notifications without any correlation to the "start IDLE" request. NOTIFY seems to work the same way.

I can't really see how either of these protocols could fit into the current API, or how to implement a specialized version for either.

@djc
Copy link
Contributor

djc commented May 19, 2017

@koivunej are you working on a tokio-based IMAP project? Or is this about your eventstore project?

My idea so far is that for IMAP, I'd probably have to implement a custom Multiplex-like setup, where I have unbounded futures::sync::mpsc::Channels for each request ID and then also a separate one for server-originated messages. I think I'll see if I can implement something like that, and maybe after that we can see if it's possible to make this part of tokio-proto somehow.

@koivunej
Copy link

@djc I'm interested in for eventstore-tcp, just took a look at the IMAP RFCs in interest of this issue and finding common ground.

My idea so far is that for IMAP, I'd probably have to implement a custom Multiplex-like setup, where I have unbounded futures::sync::mpsc::Channels for each request ID and then also a separate one for server-originated messages.

This is almost exactly what I've been sketching past days but... My rust-fu is only at the level of hardly making use of tokio, nowhere near actually designing useful APIs :) I'd like to try to help out if I can so feel free to ping me on this topic.

@djc
Copy link
Contributor

djc commented May 25, 2017

@koivunej so I looked over your #148, and independently was also wondering why your RequestIdSource thing does not use the Iterator interface. You've said here that you wanted to allow the library user to inspect the message and that you wanted to allow them to select the Uuid used, but I don't quite understand how these requirements prohibit you from using Iterator rather than inventing another Iterator-like interface. Can you give some more context?

@koivunej
Copy link

Can you give some more context?

Sure, I'll try but I didn't really spare much thought for this one but focused on getting the code compile and hopefully getting some comments on it. The pre-#148 code does not have any handling for integer overflows with the u64 counter which made I guess allowed me to just glance over it and add the functionality I needed (selecting the id).

The first problem with changing the generation part to Iterator is that the Iterator of course has Option<Self::Item>. By accepting an Iterator there should probably be some mechanism for repoting that "the id source has been depleted". This could probably be a std::io::Error::new(std::io::ErrorKind::Other, "Request id source depleted")?

You've said here that you wanted to allow the library user to inspect the message and that you wanted to allow them to select the Uuid used [...]

How would this work with Iterator based approach at client.rs?

@carllerche
Copy link
Member

Sorry for the delay.

As a result of the Tokio RFC, tokio-proto is going to be significantly simplified. As such, a generic RequestId is out of scope. I'm going close this issue.

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

No branches or pull requests

4 participants