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.
Matthias Radestock