Remove tokio-codec dependency from multistream-select. #1203
Conversation
romanb
force-pushed
the
multiselect-part-1
branch
2 times, most recently
from
e59f485
to
a44d218
Compare
In preparation for the eventual switch from tokio to std futures. Includes some initial refactoring in preparation for further work in the context of libp2p#659.
romanb
force-pushed
the
multiselect-part-1
branch
from
1061f62
to
05e3333
Compare
| let mut msg = BytesMut::new(); | ||
| request.encode(&mut msg)?; | ||
| match self.inner.start_send(msg.freeze())? { | ||
| AsyncSink::NotReady(_) => Ok(AsyncSink::NotReady(request)), |
There was a problem hiding this comment.
Not great for performances that we're dropping the encoded message if the socket is not ready, but this will no longer be a problem with new futures, so I'm ok ignoring that.
There was a problem hiding this comment.
For what it's worth, in new futures you have to wait for the Sink to be ready before calling start_send. Calling start_send is then guaranteed to never fail for readiness-related reasons.
There was a problem hiding this comment.
Just curious: with the current futures, do you see a better way for the general case of wanting to nest Sinks with different item types (due to being different layers of abstraction) while avoiding the re-encoding from the upper to the lower-level item type? I guess if the encoding would be expensive, an additional buffer of encoded messages could do? In any case, I obviously didn't think this an important issue here since the encoding is presumably cheap and from my understanding typically at most done twice for 1 out of N > 1 messages, assuming the sinks can generally buffer quite a few messages to send before being NotReady. Good to see though that this seems to be a non-issue in the newer futures API.
There was a problem hiding this comment.
with the current futures, do you see a better way for the general case of wanting to nest Sinks with different item types (due to being different layers of abstraction) while avoiding the re-encoding from the upper to the lower-level item type?
I don't think so. That's presumably why it has been changed in new futures.
| fn encode(&self, dest: &mut BytesMut) -> Result<(), MultistreamSelectError> { | ||
| match self { | ||
| Request::Protocol { name } => { | ||
| if !name.as_ref().starts_with(b"/") { |
There was a problem hiding this comment.
I'm not aware of this rule! Do protocol names have to start with /?
There was a problem hiding this comment.
Well, strictly speaking no, it is just a convention, the "spec" says
[..] While any string can be used, the conventional format is a path-like structure containing a short name and a version number, separated by / characters. [..]
but it doesn't even bother saying what a "string" here is in terms of encoding, so it sounds like a sentence quickly extrapolated from a go or js implementation. Practically, the requirement for the leading / did not change in this PR - this "restriction" was already there and is needed for decoding. Personally, I'm very much in favor of stricter parsing of protocol names reflecting the "conventional format", based on a simple grammar (e.g. a protocol name must start with a /, followed by a sequence of alphanumeric strings including dots and hyphens, separated by /).
There was a problem hiding this comment.
[..] While any string can be used, the conventional format is a path-like structure containing a short name and a version number, separated by / characters. [..]
but it doesn't even bother saying what a "string" here is in terms of encoding
I stand corrected, it does state further down that
Messages are sent encoded as UTF-8 byte strings, [..]
which implies utf-8 encoding for protocol names. This is unfortunately also not enforced by this implementation since the interface rests on AsRef<[u8]>.
In preparation for the eventual switch from tokio to std futures, this PR removes the
tokio-codecdependency frommultistream-select. This includes some initial refactoring / cleanup in preparation for further work onmultistream-selectin the larger context of #659 - I'm just trying to work in small(er) increments to make the changes more digestible.