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

Processing asynchronously received messages. #316

Closed
4 tasks
egonspace opened this issue Sep 13, 2021 · 3 comments · Fixed by #341
Closed
4 tasks

Processing asynchronously received messages. #316

egonspace opened this issue Sep 13, 2021 · 3 comments · Fixed by #341
Labels
C: bug Classification: Something isn't working P: nice to have Priority: nice to have

Comments

@egonspace
Copy link
Contributor

Summary

https://github.com/line/ostracon/blob/dbe53cb7537246681e2d87de51000c02ece2c49a/p2p/peer.go#L408
When the channel of the reactor is full due to the above code, messages are discarded. In the case of a mempool reactor, if a message is discarded, there may be a problem that tx remains in the sender's mempool. When the channel is full, consider waiting rather than discarding the message.

Problem Definition

Proposal


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tnasu
Copy link
Member

tnasu commented Sep 13, 2021

When the channel is full, consider waiting rather than discarding the message.

Totally, I agree with you.

I'm not sure, do you mean to change all asynchronous reactor behavior? Remove the default clause?

  • mempool/reactor
  • evidence/reactor
  • blockchain/v[0,1]/reactor
  • consensus/reactor
  • statesync/reactor
  • p2p/pex/pex_reactor

If we allow blocking in receiving messages, there is no problem. But the behavior becomes synchronously reactors. If a node receives too many transactions and blocking the transactions, then the node also cannot work to do consensus/statesync/etc. We should improve behavior to take into consideration synchronously each reactor's behavior. Or do we simply allow becoming synchronously reactors?

@egonspace
Copy link
Contributor Author

egonspace commented Sep 13, 2021

I think it's meaningful because it works synchronously only when the channel is full, and before that, it works async. It would be better to act the same for all reactors.

@tnasu
Copy link
Member

tnasu commented Sep 14, 2021

OK, let's remove the default clause.

@torao torao added C: bug Classification: Something isn't working P: nice to have Priority: nice to have labels Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: bug Classification: Something isn't working P: nice to have Priority: nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants