This was a convenient but rather suspect coupling. In particular, it
will break if the period gossip logic changes s.t. it no longer sends
gossip on all connections every interval.
We were invoking AddConnection in a separate goroutine, and this could
result in RemoveConnection getting invoked first, resulting in bogus
entries in our connection map, which would effectively prevent
connectivity to the other peer until the stale entry got removed
through a lost tie break. And if, per chance, the remote end ran into
exactly the same race condition, then connectivity between that peer
and ourself was broken permanently.
Hitting this race condition required a combination of a) no UDP
heartbeat being received for HeartbeatTimeout(30s) - this causes the
actorLoop to terminate and RemoveConnection to be invoked, and b) the
AddConnection invocation getting delayed for at least that long.
The previous scheme set the reference to main channel to nil and then
drained it. That was racy and hence could lead to deadlock in the
following scenario:
1. multiple senders obtain the main channel from the stored reference
2. main channel reference is set to nil
3. main channel is drained
4. multiple senders send to main channel
So the issue here is that the entire forwarder stopping sequence
occurs between senders obtaining of the reference to the main channel,
and then sending on that channel. If more senders sent than there is
capacity in the channel then this leads to deadlock.
Treat the no-error case the same as the error case...
- use the standard sendAction to communicate with the actor, which
ensures that we don't block when the actor has terminated
- run in a goroutine to prevent delaying the caller when the actor's
channel is backed up.
The only slight trickiness is Shutdown. Generally connections shut
down when an error is encountered, but we also need to be able to shut
down connections w/o an error. Rather than making all the actor
methods return a (terminated bool, error) tuple, we signal the
non-error shutdown by sending nil as the action (instead of a
closure).
There were situations, such as deleting another connection as result
of it losing a tie-break, where lifecycle events of connections on the
local peer weren't logged.
Fixing that entails moving the logging closer to (i.e. right after)
the state change action. This in turn requires promotion of logging to
the Connection interface instead of it LocalConnection. Which in turn
requires a change from 'log' to 'Log', for consistency with other
interface methods.
Fixes#449.
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
Matthias Radestock