ts_tunnel: give up on handshake initiation after enough retries - #381
Conversation
Updates #339 Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: Ibf8819c3cff3c3916666135969bce9a26a6a6964
9907fe7 to
0fdf5ed
Compare
| return; | ||
| match self.handshake.timeout(endpoint, &self.config, now) { | ||
| Ok(packet) => out.queue_to_peer(self.config.id, [packet]), | ||
| Err(TimeoutError::WrongHandshakeState) => {} |
There was a problem hiding this comment.
this only happens if we're fighting with the peer for the initiator role, right (assuming no bugs in the state machine)? e.g. we initiate, the peer sends us back an initiation packet, so we become the receiver, but the timeout event isn't canceled, so it fires even though we're not initiator anymore.
There was a problem hiding this comment.
Hmm. I was going to say that it could happen either if the endpoint transitions from the initiator to the responder role, or if the handshake completion races with the timeout firing.
However, I think both of those cases should be covered by cancellation, because SentHandshake holds the handle for the timeout event. So, any transition to another handshake state would cancel the timeout event.
This makes me tempted to turn that defensive error into a hard assert that it never happens, but I'm not sure if I'm confident enough that we'll never go through the timeout codepath in a surprising state.
I'll merge this as-is on the basis that defensively doing nothing here is also correct, just maybe unnecessary. And I'll ponder things further and see if I can convince myself that it can be a hard invariant assertion instead.
There was a problem hiding this comment.
The only other pathway to this case I see is if it were possible for two threads to run simultaneously, one handling recv and changing the handshake state, and the other doing dispatch_events and trying to act on the timeout event. The way the locking happens within Endpoint, the handling of the event and the handshake state change could interleave and lead to trying to handle a timeout for the wrong handshake state.
However, currently that is prevented by all those external methods taking &mut self. In the longer term if we want multiple parallel dataplanes, Endpoint will have to acquire more interior mutability and then we'll have to worry more about that scenario... But right now I think rust's borrowing rules guarantee that the existence of the timeout event is mutually exclusive with the handshake being in a different state.
2 participants
Updates #339
Change-Id: Ibf8819c3cff3c3916666135969bce9a26a6a6964
This fixes the surface finding of #339. I'm not marking the issue fixed yet, because I believe there's a related issue with how handshakes in the responder role never timeout, which in a particular sequence of events could leave the responder deadlocked and unable to switch to the initiator role. It's a separate issue though that I'm going to deal with separately.