Actively Deduplicate Connections on WaitForCapabilityOrClose
continuous-integration/drone/push Build is pending Details
continuous-integration/drone/pr Build is pending Details

A very rare bug happens when 2 contacts peer with each other at the same
time. This results in duplicate higher level constructs like PeerApp
which can make tracking state-related bugs difficult, especially
in integration tests.

This commit fixes an existing bug in WaitForCapabilityOrClose
which hid the existence of a duplicate connections from clients
(and replaces it with active deduping)
This commit is contained in:
Sarah Jamie Lewis 2021-09-08 11:53:39 -07:00
parent 6e7fcad7a6
commit c19b1011ee
1 changed files with 26 additions and 7 deletions

View File

@ -10,7 +10,6 @@ import (
// BaseOnionService is a concrete implementation of the service interface over Tor onion services.
@ -66,16 +65,36 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab
conn, err := s.GetConnection(cid)
// If there is at least one active connection, then check the one that is returned and use it
for conn != nil {
if conn.HasCapability(name) {
// if we have only one and it has the right capability then return
if conn.HasCapability(name) && err == nil {
return conn, nil
time.Sleep(time.Millisecond * 200)
// If we received both a Connection and an Error from GetConnection that means we have multiple connections
// If that was the case we refresh from the connection store as the likely case is one of them has been
// closed and the other one has the capabilities we need.
// If We have 2 connections for the same hostname...
if err != nil {
conn, err = s.GetConnection(cid)
log.Debugf("found duplicate connections for %v <-> %v",, cid)
// By convention the lowest lexicographical hostname purges all their outbounds to the higher
// hostname
// Which should only leave a single connection remaining (as we dedupe on connect too)
// This does allow people to attempt to guarantee being the outbound connection but due to the bidirectional
// authentication this should never result in an advantage in the protocol.
if < cid {
// Close all output connections to connection
s.connections.Range(func(key, value interface{}) bool {
connection := value.(tapir.Connection)
if connection.Hostname() == cid {
if !connection.IsClosed() && connection.IsOutbound() {
return true
// We had a connection but it didn't have a cpability...try again
conn, err = s.GetConnection(cid)
// There are no active connections with that hostname
return nil, err