Remove support for multiple network protocols #2005
Conversation
|
This slightly modifies what the |
| @@ -77,8 +77,8 @@ impl<TMessage, TSubstream> Behaviour<TMessage, TSubstream> { | |||
| /// Also note that even we have a valid open substream, it may in fact be already closed | |||
| /// without us knowing, in which case the packet will not be received. | |||
| #[inline] | |||
There was a problem hiding this comment.
Might be worth updating the comment here as well.
|
|
||
| /// Topology of the network. | ||
| topology: NetTopology, | ||
|
|
||
| /// List of custom protocols that we have open with remotes. | ||
| open_protocols: Vec<(PeerId, ProtocolId)>, | ||
| opened_peers: FnvHashSet<PeerId>, |
| @@ -302,11 +288,10 @@ impl<TMessage, TSubstream> CustomProtos<TMessage, TSubstream> { | |||
| /// | |||
| /// Also note that even we have a valid open substream, it may in fact be already closed | |||
| /// without us knowing, in which case the packet will not be received. | |||
| pub fn send_packet(&mut self, target: &PeerId, protocol_id: ProtocolId, message: TMessage) { | |||
| pub fn send_packet(&mut self, target: &PeerId, message: TMessage) { | |||
| )); | ||
| let event = CustomProtosOut::CustomMessage { | ||
| CustomProtoHandlerOut::CustomMessage { message } => { | ||
| let event = CustomProtoOut::CustomMessage { |
There was a problem hiding this comment.
You could keep the debug assert as self.is_open(&source)
There was a problem hiding this comment.
There are two more removed asserts nearby - in CustomProtoHandlerOut::CustomProtocolOpen (opposite assert) && in CustomProtoHandlerOut::Clogged (the same assert). Probably worth keep these as well?
| /// different enum that doesn't contain any `protocol_id`, and the caller would inject | ||
| /// the ID itself, but that's a ton of code for not much gain. | ||
| fn poll(&mut self) -> Option<CustomProtoHandlerOut<TMessage>> { | ||
| for n in (0..self.pending_response.len()).rev() { |
There was a problem hiding this comment.
Could be while let Some(_) = self.pending_response.pop() {? Any advantages of this approach that I'm missing?
There was a problem hiding this comment.
We're calling self.pending_response.push(...) within the loop. By doing it this way we only process each element once.
| } | ||
| }, | ||
| Ok(Async::Ready(Some(RegisteredProtocolEvent::Clogged { .. }))) => | ||
| unreachable!("Cannot receive Clogged message with new protocol version; QED"), |
There was a problem hiding this comment.
As I'm reading the entire code for the first time it seem that it could be a bit more elaborate, might be good to give some hints (roughly) what parts of the code guarantees that.
There was a problem hiding this comment.
I was going to add a code comment, but it would be redundant with the message. Basically there's a backwards-compatible version of the protocol and a new version that we're going to enable in the future. The new version doesn't generate Clogged events.
There was a problem hiding this comment.
Removing the Clogged event is the next step in #1692.
| let mut substream = self.incoming_substreams.swap_remove(n); | ||
| match substream.poll() { | ||
| Ok(Async::Ready(Some(RegisteredProtocolEvent::Message(message)))) => { | ||
| if let CustomMessageId::Request(id) = message.request_id() { |
| }); | ||
| } else { | ||
| self.shutdown.push(substream); | ||
| return Some(CustomProtoHandlerOut::ProtocolError { |
There was a problem hiding this comment.
You could return the entire if-else expression, would be clearer that this branch always returns.
| shutdown_list(&mut self.shutdown); | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Just noticed that entire code was just moved, so feel free to disregards the comments.
|
Telemetry currently is completely agnostic on what network state JSON is. |
* Remove support multiple network protocols * Address concerns * Add back debug_asserts
In preparation for the addition of the peerset, this PR removes support for multiple custom protocols.
In practice we were only ever using one anyway.
Also in practice I don't think multiple protocols were working. It's an untested feature, and I can think of a few possible situations that we may not handle properly.
Support for multiple protocols can be added back in the future if there's a need.
While the diff is quite large, it is mostly renames, removing the
protocol_idfield, and moving a few functions around. No logic has been touched.