Topology gossip can be received on a connection before that connection
has been marked as 'established'. If we do not include unestablished
connections in topology gossip, then parts of the topology can be
removed by gc, since they are deemed unreachable, which can cause
subsequent incremental topology gossips to be rejected due to them
containing references to peers that have been removed by the gc.
Including unestablished connections in topology gossip requires:
- removing the check for 'established' connections in the encoding of
peers for topology gossip,
- adding the 'established' flag to the internal representation of
remote connections,
- adding the 'established' flag to to encoding of connections in
topology gossip,
- treating the *addition* of a connection as an update to the local
peer.
The latter entails
- bumping the peer version on connection addition (as well as on
marking 'established' as previously),
- bumping the peer version *unconditionally* on connection deletion
(rather than only when the connection was 'established', as
previously),
- moving the initial sending of complete topology on the connection,
and the broadcasting of a topology update containing the two peers
at the ends of the connection, from the connection 'established'
logic to the connection addition logic,
- broadcasting just an topology update for the local peer when a
connection gets marked as 'established',
- broadcasting a topology update for the local peer *unconditionally*
when a connection is removed (rather than only doing this when the
connection was 'established', as previously).
Caused by a race between adding the connection to our map and
receiving a topology gossip on it.
We remove the race by making AddConnection synchronous and invoking it
prior to entering the tcp receive loop.
Fixes#367.
Prior to the introduction of gossip, a tcp connection would not see
regular communication until it had been marked as 'established',
because the topology communication scheme would periodically send
'FetchAll' only after that. So in the absence of udp connectivity we'd
time out. The timeout could of course occur for other reasons, namely
disrupted tcp connectivity, but the combination of a timeout and 'not
established' pointed to a lack of udp connectivity as a likely
cause. So that's what we checked for.
In the new gossip-based topology communication, however, gossip (of
anything, not just topology) occurs as soon as we have tcp
connectivity. Therefore we will no longer encounter a timeout when
just udp connectivity is disrupted. Which makes our check invalid.
So instead we introduce a one-shot timer, created on handshake
completion and cancelled when we mark the connection as
'established'. When the timer fires, and the connection still hasn't
been established, we shut down the connection with an error indicating
lack of udp connectivity.
The router may receive heartbeat packets that originated from (the
other end of) a different connection with the remote peer than the
current one. If we don't detect that, we may erroneously tell the
remote peer to mark the connection as 'established'. Which can lead to
a crash.
To enable detection, we include the connection UID - which is the same
at both ends - in the heartbeat frame. That way we can simply ignore
heartbeats with a different connection UID than the current connection
to the remote peer.
Previously heartbeat frames were zero length. In order to include the
64 bit connection UID we could just send 8 octets instead. However,
that would potentially trip up the Ethernet decoder at the other end,
and in any case we might as well make heartbeats fit the format of
other special frames, namely having an all-zero Ethernet header. This
in fact results in a simplification of the special frame handling,
since heartbeats are no longer *extra* special.
Fixes#352.
1) stop the fast heartbeater asap; reducing the liklihood of getting
an extraneous beat
2) uniformely deal with 'avoiding initial wait for timer ticks'; this
entails sending a FetchAll immediately
3) because of (2) there is now no point initiating a FetchAll from the
tcp reader
Currently we cannot distinguish bi-directional and uni-directional
connectivity between peers. This is what currently happens for a
connection from A to B...
A: remoteUDPAddr=X
B: remoteUDPAddr=nil
A: send fast heartbeats over UDP
B: receive heartbeat; remoteUDPAddr is nil
remoteUDPAddr=Y
send 'established' over TCP
established=true
send slow heartbeats over UDP
A: receive 'established' over TCP
established=true
send slow heartbeats over UDP
If udp packets make it through from A to B but not B to A then both
ends mark the connection as 'established'. Conversely, if udp packets
make it through from B to A but not A to B then the connection is not
marked as 'established' at either end.
In the new scheme, this is what happens...
A: receivedHeartbeat=false, remoteUDPAddr=X
B: receivedHeartbeat=false, remoteUDPAddr=nil
A: send fast heartbeats over UDP
B: receive heartbeat; receivedHeartbeat is false
remoteUDPAddr=Y
receivedHeartbeat=true
send 'established' over TCP
send fast heartbeats over UDP
A: receive 'established' over TCP (*)
established=true
send slow heartbeats over UDP
A: receive heartbeat; receivedHeartbeat is false (*)
receivedHeartbeat=true
send 'established' over TCP
B: receive 'established' over TCP
established=true
send slow heartbeats over UDP
(*) either of these may occur first
Now, if udp packets make it through from A to B not not B to A then A
marks the connection as 'established' but B doesn't. Conversely, if
udp packets make it through from B to A but not A to B then B marks
the connection as 'established' but A doesn't.
1) log more lifecycle events
- acceptance
- handshake completion
- added & removed (from our connection map)
The latter is a bit verbose but may help us track down the cause for
going to introduce here, then this would fall in the 'debug' category.
2) better error logging
- log error as part of "shutting down" log event rather than separately
- indicate "blocked UDP connectivity" as a possible cause when
getting a timeout on a not-fully-established connection.
The rationale for the latter is that inbound connections get
marked as established when first receiving a heartbeat frame over
UDP, which are the only frames that get transmitted prior to the
connection being fully established. Note that outbound
connections get marked as established when receiving the
ConnectionEstablished message over TCP, which is sent by the peer
when it first receives a heartbeat. See #342.
Closes#340.
conn.shutdown was only set/accessed in handleShutdown, which in turn
is only called on termination of queryLoop and hence multiple
invocations are impossible.
There is no need to go via Topology. Fetching does not access or
modify the Topology state, and none of the functions on Topology
update the PeerCache. So there is no point serialising the fetching
with RebuildRoutes (or anything else).
This does mean the connections' receiveTCP goroutines will take a
direct lock on PeerCache, but a) it is only a read lock, and b) they
already take a write lock in another code path.
There may be some extra load on the PeerCache as a result of this
change, due to the topology fetching of multiple connections no longer
being funneled through the single Topology goroutine. Consequently,
write locks on the PeerCache may take a little longer to
acquire. Until we encounter this as a genuine problem though it's
better to stick with the simpler code.
The weave protocol already effectively performs heartbeating -
FetchAll is performed at regular intervals. Hence we can detect stale
connections by the lack of receiving any data for approximately that
interval (we use x2).
We use the same interval to set a time limit on the handshake too, and
any subsequent interactions prior to the connection being fully
established.
Fixes#272.
...by moving the logic into the connection actor goroutine.
The heartbeater goroutine was doing a lot more than just heartbeating;
namely it also had tickers for triggering a ProtocolFetchAll and
initiating a fragmentation test. Furhermore, ensureHeartbeats also
spawned the forwarders.
So this was quite a mix of functionality, and, furthermore, most of it
related quite closely to the connection actor. So moving the
functionality into that simplifies the code.
In the process we also fix#294.
Matthias Radestock