Ensure peer_disconnected is called after a handler refuses a connection
#3580
Conversation
If one message handler refuses a connection by returning an `Err` from `peer_connected`, other handlers which already got the `peer_connected` will not see the corresponding `peer_disconnected`, leaving them in a potentially-inconsistent state. Here we ensure we call the `peer_disconnected` handler for all handlers which received a `peer_connected` event (except the one which refused the connection).
...to make it identical to all our other message handlers.
This adds a `ConnectionTracker` test util which is used across
`TestChannelMessageHandler`, `TestRoutingMessageHandler` and
`TestCustomMessageHandler`, asserting that `peer_connected` and
`peer_disconnected` methods are well-ordered. This expands test
coverage from just `TestChannelMessageHandler` to cover all test
handlers and adds some useful features which we'll use to test
the fix in the next commit.
This also adds an additional test which tests
`peer_{dis,}connected` consistency when a handler refuses a
connection by returning an `Err` from `peer_connected`.
There was a problem hiding this comment.
Should we enforce uniformity going forward by having the respective handler interfaces inherit from a general trait PeerMessageHandler interface and move peer_{connected, disconnected} and provided_{node,init}_features to it?
Note this would also allow us to iterate over all previously-succeeded handlers rather than having to call them individually (which might get stale at some point in the future).
There was a problem hiding this comment.
Yea, we should definitely require uniformity going forward. We could definitely do a higer-level interface, do you want it in this PR or when we add the next handler?
There was a problem hiding this comment.
We could definitely do a higer-level interface, do you want it in this PR or when we add the next handler?
I'd have a slight preference to do it right away, but feel free to do in a follow-up if you'd rather land this quickly.
There was a problem hiding this comment.
I wrote the patch, but its 36 files changed, 982 insertions(+), 1018 deletions(-), so kinda would prefer to split it up just for sanity.
|
Gonna go ahead and land this as both reviewers said LGTM and don't have remaining comments. |
This is similar to #3110 but does the disconnection calls inline to avoid the races described in the discussion there.
Only the first commit should be backported.