client/finality-grandpa: Make round_communication use bounded channel #4691
Conversation
`round_communication` returns a `Sink` and a `Stream` for outgoing and incoming messages. The messages send into the `Sink` are forwarded down to the network as well as send back into the `Stream` to ensure the node processes its own messages. So far, to send messages into the `Sink` back into the `Stream`, an unbounded channel was used. This patch updates `round_communication` and `OutgoingMessages` to use a bounded channel. This is part of a greater effort to reduce the number of owners of components within `finality-grandpa` and `network` as well as to reduce the amount of unbounded channels. For details see d4fbb89 and f0c1852.
Correct. The channel being bounded in this patch is used to send packets back to our self. Thus we depend on us reading from the channel. |
|
Let's maybe merge #4612 first? |
mxinden
changed the title
|
@mxinden We need to ensure that we don’t need to buffer an unbounded amount of data. |
|
I agree with the theory behind this change. In practice though I think this is bound to cause more problems than it aims to solve. In particular because AFAIK going OOM (or any noticeable memory usage increase) due to these unbounded channels has not been a problem so far. I'd appreciate that this is properly tested before it is rolled out on a live network to avoid surprises (maybe by deploying to flaming fir along with #4612). |
There was a problem hiding this comment.
Changes lgtm. Regarding my earlier comment on any issues that might be introduced by this, it was made before checking which unbounded channel was being changed. After reviewing the PR I think this one is not problematic at all. Still agree with Pierre that this could probably be merged after #4612 so that we don't need to mix futures versions.
@demimarie-parity I don't understand your comment. Yes, we need to make sure to not buffer unbounded amount of data. That is what the effort behind these pull requests tries to achieve. In the end you can see an unbounded channel as an unbounded buffer. |
|
I would prefer not to block this pull request on #4612 given that the latter is quite large and given that it is unclear when it will be merged.
We already mix both futures01 and futures03 within The changeset in this pull request is not large and I am happy to resolve the merge conflicts with #4612 (already half done locally) once this one merged. @tomaka @andresilva @expenses let me know if this would be ok for you. (I will look into #4612 and #4633 next and try to help out.) |
|
@mxinden I doubt the merge conflict would be especially difficult, so that works for me! |
| match decoded { | ||
| Err(ref e) => { | ||
| debug!(target: "afg", "Skipping malformed message {:?}: {}", notification, e); | ||
| return ready(None); |
There was a problem hiding this comment.
I'd prefer if this read future::ready instead. Wasn't immediately obvious to me.
There was a problem hiding this comment.
I agree. As a rule of thumb, importing free functions into the local namespace hurts readability.
module::function lets us know where to look for the function without searching the imports.
function lets us know that the function is in the current module.
There was a problem hiding this comment.
Good point. Fixed with 6858319.
After #4612 we don't, which is what I meant. I'm OK with not blocking this PR though. |
Instead of importing futures03::future::ready into the scope, only import futures::future03 into scope and call ready as furure03::ready.
|
Thanks for the reviews and help! @rphmeier any further comments from your side? |
round_communicationreturns aSinkand aStreamfor outgoing andincoming messages. The messages send into the
Sinkare forwarded downto the network as well as send back into the
Streamto ensure the nodeprocesses its own messages.
So far, to send messages into the
Sinkback into theStream, anunbounded channel was used. This patch updates
round_communicationandOutgoingMessagesto use a bounded channel.This is part of a greater effort to reduce the number of owners of
components within
finality-grandpaandnetworkas well as to reducethe amount of unbounded channels. For details see d4fbb89 and
f0c1852.