Keep state consistent during disconnection #729
Conversation
vlad9486
force-pushed
the
fix/disconnection
branch
2 times, most recently
from
3757dc8 to
5bbff1b
Compare
| } | ||
| } | ||
| MioEvent::ConnectionDidCloseOnDemand(addr) => { | ||
| SubStore::dispatch(store, P2pNetworkSchedulerAction::Prune { addr }) |
There was a problem hiding this comment.
maybe u should dispatch P2pDisconnectionAction::Finish as well?
There was a problem hiding this comment.
In fact I'd put SubStore::dispatch(store, P2pNetworkSchedulerAction::Prune { addr }) action in effects of the P2pDisconnectionAction::Finish
There was a problem hiding this comment.
It is too big change and require more debugging to make it work properly. Let's allow connection where conn_state.closed is Some(..) to exist after Finish done.
vlad9486
force-pushed
the
fix/disconnection
branch
14 times, most recently
from
0b134e2 to
bff3ab4
Compare
vlad9486
force-pushed
the
fix/disconnection
branch
5 times, most recently
from
cbd713f to
cee0306
Compare
vlad9486
force-pushed
the
fix/disconnection
branch
from
cee0306 to
c6eb044
Compare
| // enabling condition | ||
| // it could be the case that the connection initialized again, so should not remove the state | ||
| let exist = store.state().p2p.ready().unwrap().peers.iter().any(|(_, x)| match &x.dial_opts { | ||
| Some(p2p::connection::outgoing::P2pConnectionOutgoingInitOpts::LibP2P(o)) => o.matches_socket_addr(addr.sock_addr), |
There was a problem hiding this comment.
For outgoing connections, socket address could match but it could be a new connection and this could be a stale action.
There was a problem hiding this comment.
To properly fix this, it might require bigger changes, so maybe this is fine for now.
There was a problem hiding this comment.
Oh, I just notice the unwrap, I remove it now.
| Some(p2p::connection::outgoing::P2pConnectionOutgoingInitOpts::LibP2P(o)) => o.matches_socket_addr(addr.sock_addr), | ||
| _ => false, | ||
| }); | ||
| if !exist { |
There was a problem hiding this comment.
This block will be executed if it's a webrtc connection, or if libp2p peer doesn't have the same socket addr. Is that the intended behavior? Maybe it was supposed to be just if exist {?
There was a problem hiding this comment.
I should rename exist -> ready. When we removing connection, the peer should be in Disconnecting state, not Ready(..). In webrtc case MioEvent will not happen, so it is irrelevant.
To properly handle the case when new connection is initializing during old connection is still Disconnecting, maybe we need a counter in the state or in the key, in this map:
pub peers: BTreeMap<PeerId, P2pPeerState>,
but anyway, disconnection is a millisecond at worst, and reconnection timeout is much bigger, so it should not be possible.
| addr, | ||
| reason: reason.clone(), | ||
| }); | ||
| dispatcher.push(P2pDisconnectionAction::Finish { peer_id: *peer_id }); |
There was a problem hiding this comment.
If it gets insta finished, without any event from mio, then disconnecting state seems useless.
There was a problem hiding this comment.
Let's keep it for further improvement. I will make Finish an effect of Prune, but not now.
3 participants
Delay removal of the connection from the network state machine until the disconnection actually occurs at the TCP level.