Codecov Report

Merging #5671 (8ee0d8c) into master (89e908e) will increase coverage by 0.06%.
The diff coverage is 59.34%.

@@            Coverage Diff             @@
##           master    #5671      +/-   ##
==========================================
+ Coverage   60.50%   60.57%   +0.06%     
==========================================
  Files         259      261       +2     
  Lines       23345    23618     +273     
==========================================
+ Hits        14126    14306     +180     
- Misses       7743     7829      +86     
- Partials     1476     1483       +7     

Occurred to me that we'll need better panic handling here too. In this PR, a panic during message processing causes the entire channel processing goroutine to terminate. In the current P2P stack, a panic causes the peer to be disconnected. We should probably disconnect the peer but otherwise continue message processing, similarly to how a web-server doesn't terminate if a HTTP request panics.

Occurred to me that we'll need better panic handling here too. In this PR, a panic during message processing causes the entire channel processing goroutine to terminate. In the current P2P stack, a panic causes the peer to be disconnected. We should probably disconnect the peer but otherwise continue message processing, similarly to how a web-server doesn't terminate if a HTTP request panics.

Agree, but this is dependent on the individual reactors. Specifically, for state-sync, we do not have any explicit panic invocations. However, this doesn't mean a panic can't happen (e.g. nil pointer). Are you suggesting we have a recover block? If so, we should devise a way where each reactor doesn't have to do this themselves (if possible, maybe not).

Agree, but this is dependent on the individual reactors. Specifically, for state-sync, we do not have any explicit panic invocations. However, this doesn't mean a panic can't happen (e.g. nil pointer). Are you suggesting we have a recover block? If so, we should devise a way where each reactor doesn't have to do this themselves (if possible, maybe not).

Yeah, it follows from using channels that it's the consumer's responsibility to handle panics and other errors during processing. Since we're receiving messages from random people on the Internet here, we need to be resistant to adversarial inputs, and that includes panic recovery.

There's a bunch of different options here, for example:

1. Move message processing into a handleMessage(from p2p.PeerID, msg proto.Message) error method and do panic recovery there or in the caller (this also improves rightwards code drift and error handling).

2. Add a convenience method like Channel.Process(context.Context, func (Channel, p2p.Envelope) error) error that consumes the channel and calls the callback for each message until the context (or closer or whatever) is cancelled, handling panics and other issues.

3. Keep running the channel processing method in a loop until the reactor is shut down.

These are just a few ideas off the top of my head, there's probably other good options as well. I think this mostly depends on what's most convenient, understandable, and flexible so it's probably a good idea to experiment with a few approaches and see what works best.

I went with something that is more like (1) because I do not want to make any premature abstractions or optimizations without knowing how all the reactors will look like. This is something we can cleanup in the future in, most likely, a non-breaking manner.

So now we have a simple handleMessage that handles panic recovery. handleMessage will then handle the message accordingly.