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

async send method for network trait to handle backpressure #331

Closed
wants to merge 1 commit into from

Conversation

joschisan
Copy link
Contributor

No description provided.

@github-actions
Copy link

Please make sure the following happened

  • Appropriate tests created
  • Infrastructure updated accordingly
  • Updated existing documentation
  • New documentation created
  • Version bumped if breaking changes

@ghost ghost added the pending label Aug 25, 2023
Copy link
Contributor

@fixxxedpoint fixxxedpoint left a comment

Choose a reason for hiding this comment

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

It looks ok in general. I don't know what exactly is your goal with this change, but please notice that it doesn't actually create much backpressure on other components, since the main component that uses this trait uses unbounded_channels anyway. So the only backpressure I see here is delays between unit messages and alerts being send. This can also create another type of problem, i.e. a single laggy node can slow down significantly communication with other nodes. In our implementation of this trait, we've decided to use some backpressure only on the receiver side simply by introducing a rate-limiting wrapper.

consensus/src/member.rs Outdated Show resolved Hide resolved
fuzz/src/lib.rs Outdated Show resolved Hide resolved
@joschisan
Copy link
Contributor Author

The goal with this change is to only drop messages if necessary in one place - the network layer if a peer is offline or laggy. However, I also do not want to drop messages when calling a sync send method on the network layer and dropping it just because the queues just happen to be full then. Furthermore, it could allow the network layer to run async code in its send method, which may be useful depending on the implementation. Its a small change, but allows for a cleaner system imo.

@joschisan
Copy link
Contributor Author

Please let me know how to bump the version numbers for this pr.

@joschisan joschisan closed this Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants