We did this for symmetry/completeness, but the encoder is actually
"consumed" by handshake() in the creation of the tcp senders, hence it
is in fact dangerous to return it.
We now first exchange a very minimal header, consisting of just the
name of the protocol ("weave") and the min and max version (encoded as
a byte each) we support. This leaves all other details of the
protocol, including the contents of of the handshake, open.
Peers agree to communicate using the maximum mutually supported
version. We record this in the LocalConnection, though atm nothing is
making use of that information.
We set a shorter than normal deadline (10s instead of 60s) for the
header exchange, so we detect version mismatches, garbage, or
non-responsive remotes quickly.
We reset the protocol version to '1' since this is a fresh start; old
weaves will not be able to parse the header.
Closes#871.
This is how fragmentation tests used to happen:
A: conn.stackFrag = false
A->B: StartFragmentationTest
B->A: send large non-DF UDP packet
A->B: FragmentationReceived
B: conn.stackFrag = true
conn.stackFrag = true means "UDP packets sent to the other peer get
correctly fragmented and re-assembled".
Note how A clears its own stackFrag flag, but the test establishes
whether fragmentation works for B->A, and ends up setting stackFrag on
B. Not only is this weird, it also means that if A previously had
stackFrag=true, it will now start fragmenting packets until B
initiates a fragmentation test.
The new sequence is:
A: conn.stackFrag = false
A->B: send large non-DF UDP packet
B->A: FragmentationReceived
A: conn.stackFrag = true
Closes#746.
From the golang spec: 'A "break" statement terminates execution of the
innermost "for", "switch", or "select" statement within the same
function.' But the LocalConnection actorLoop seems to assume it
skips the "select" and terminates the enclosing "for".
As a result, if something did conn.Shutdown(nil), it would not actually
shutdown the connection. It turns out this is never a problem, because
Shutdown is always called with a non-nil error, even though there is code
in a few places to handle the nil case. By cleaning that up, the dubious
"break" is eliminated.
This makes for a more symmetric API - Peers.FetchWithDefault does the
increment and Peers.Dereference does the decrement, whereas previously
the latter was done by Peer.DecrementLocalRefCount.
This does increase lock granularity, i.e. modifying any ref count now
requires a write lock on Peers.
When we want a string representation of a Peer, we nearly always just
want the full name. So make String() return that and introduce Info()
to return expanded info.
Since TCP is reliable, we do not need to transmit the nonce itself;
the recipient can simply construct the nonce from its own message
counter. Hence there is no message envelope and we can thus remove a
whole layer of encoding.
It is wasteful and can trip up crypto at the other end. See comment
for why.
To accomplish this we move the responsibility for shutting down the
new, invalid connection from LocalPeer.AddConnection to its caller,
i.e. LocalConnection.run. So rather than always firing up all the
connection machinery and getting told asynchronously by AddConnection
to shut it all down again, we now only fire it up if AddConnection
succeeded.
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.
Matthias Radestock