Fix muxer that can be stucked into an infinite loop

[elmattic]

Summary of changes

Changes introduced in this pull request:

• Fix muxer that can go into an infinite loop during shutdown

Reference issue to close (if applicable)

Closes #2672

82ca23c does run Muxer future separately to reproduce the issue.

Other information and links

Change checklist

• I have performed a self-review of my own code,
• I have made corresponding changes to the documentation,
• I have added tests that prove my fix is effective or that my feature works
  (if possible),
• I have made sure the
  CHANGELOG is
  up-to-date. All user-facing changes should be reflected in this document.

[lemmih]

Could you explain the problem and how this fixes it?

[elmattic]

When creating the daemon chain_muxer task, we are giving it the network_rx channel just created before.

The root cause of the problem lies in the order of cancelation of those tasks during tokio rt shutdown.

If the chain_muxer is the first to be cancelled, so before p2p_service, everything should be fine. Otherwise the network_rx channel is closed and some error could be received in the ChainMuxer promise.

My fix consists into catching this error and returning Poll::Pending to avoid continuously looping. Maybe we should test for the string "receiving on a closed channel" as well, just to be sure.

[LesnyRumcajs]

So this won't loop at all now and will exit? Or will it continue running, just without the error logs?

[elmattic]

So this won't loop at all now and will exit? Or will it continue running, just without the error logs?

This gives the hand to the tokio runtime. And since the task has be cancelled it will then exit.

[lemmih]

I still have a hard time understand both the problem and the solution. Could you explain it in a simpler way and then add that explanation to the code?

[lemmih]

This seems rather fragile to me. What if something fails with a different message? Can't we fix the underlying issue? Why can any error throw us into an infinite loop?

[elmattic]

I still have a hard time understand both the problem and the solution. Could you explain it in a simpler way and then add that explanation to the code?

I've added a comment and testing as well for the specific error string. I'm not sure if it's the best but I'm open to some better fix.

[lemmih]

I still have a hard time understand both the problem and the solution. Could you explain it in a simpler way and then add that explanation to the code?

I've added a comment and testing as well for the specific error string. I'm not sure if it's the best but I'm open to some better fix.

It looks to me that the root of the problem is that we cannot presently tell the difference between incorrect input (such as malformed tipsets) and fatal errors (such as a DB error or a closed channel). Right now, we treat all errors as recoverable and enter a tight, infinite loop if the errors are persistent.

I propose we bail on errors by default and then keep a white-list of recoverable errors (such as receiving bad data from peers). This would be a band-aid solution until we can re-write the muxer code.

[LesnyRumcajs]

I need this.

[lemmih]

Let's see if it causes any problems. It might bail too eagerly on transient errors.