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

Avoid throwing exceptions in mini protocols #2499

Closed
edsko opened this issue Aug 6, 2020 · 4 comments
Closed

Avoid throwing exceptions in mini protocols #2499

edsko opened this issue Aug 6, 2020 · 4 comments
Assignees
Labels
consensus issues related to ouroboros-consensus

Comments

@edsko
Copy link
Contributor

edsko commented Aug 6, 2020

From @coot :

The network-mux library, for some time, is capable of returning values from each mini-protocol. We would like you guys to move from throwing exceptions (which was necessary in the previous version of mux) to gracefully finish a protocol and return. AFAIK that's only important for the chain-sync client when it decides that the remote chain is too old, in which case now it should not throw, but gracefully return (i.e. reach the termination state of the protocol). A clean exit allows us to restart mini-protocols on the same bearer (what the peer-to-peer governor will do when it decides to promote the peer back as a hot one).

I think you don't need to worry about synchronisation between all the protocols, e.g. if chain-sync exists, the governor should ask all the other protocols to terminate (I need to review if it actually does it though), and it's probably the best place to handle that.

This should work with the current mux compatibility layer that we are using. The small change that we'd need to do is to handle the return value and run ErrorPolicies on it to actually suspend the peer. This means this could go to master. Alternatively, it could be just merged with coot/connection-manager, and go to master with all the other changes.

When throwing is ok? When a protocol is violated. One thing to remember is when one throws from the context of a protocol it will always bring the bearer down, and since we will run it in bidirectional mode it will shutdown both responder and initiator whether the exception was thrown from initiator or responder side.

@edsko edsko added consensus issues related to ouroboros-consensus priority medium labels Aug 6, 2020
@coot coot added the peer2peer label Aug 6, 2020
@mrBliss
Copy link
Contributor

mrBliss commented Aug 13, 2020

@coot Let me know by when you need this.

@coot
Copy link
Contributor

coot commented Aug 14, 2020

Could you prioritise it? I am about to start to integrate connection-manager & p2p-governor with consensus (in a branch). These changes will need to be merged to master together. It will likely take me some time to test it, before merging it to master.

@mrBliss mrBliss added this to the S20 2020-08-27 milestone Aug 17, 2020
@coot
Copy link
Contributor

coot commented Aug 17, 2020

@mrBliss the coot/connection-manager branch is integrated with ouroboros-consensus. Please open a PR against it.

mrBliss added a commit that referenced this issue Aug 20, 2020
coot pushed a commit that referenced this issue Aug 20, 2020
coot pushed a commit that referenced this issue Aug 20, 2020
coot pushed a commit that referenced this issue Aug 20, 2020
coot pushed a commit that referenced this issue Aug 20, 2020
coot pushed a commit that referenced this issue Aug 20, 2020
@mrBliss
Copy link
Contributor

mrBliss commented Aug 24, 2020

Can be closed as #2525 was merged.

@mrBliss mrBliss closed this as completed Aug 24, 2020
coot pushed a commit that referenced this issue Aug 25, 2020
mrBliss added a commit that referenced this issue Aug 26, 2020
coot pushed a commit that referenced this issue Aug 27, 2020
nfrisby pushed a commit that referenced this issue Aug 27, 2020
coot pushed a commit that referenced this issue Aug 27, 2020
mrBliss added a commit that referenced this issue Aug 28, 2020
mrBliss added a commit that referenced this issue Aug 28, 2020
karknu pushed a commit that referenced this issue Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

No branches or pull requests

3 participants