From 26be22510791d72c43681466b289295374bd2bfa Mon Sep 17 00:00:00 2001
From: Marco Munizaga <git@marcopolo.io>
Date: Fri, 24 Jan 2025 14:06:45 -0800
Subject: [PATCH] Sequential upnp queries

---
 p2p/net/nat/internal/nat/nat.go | 59 +++++++++++++++++----------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/p2p/net/nat/internal/nat/nat.go b/p2p/net/nat/internal/nat/nat.go
index 7d4c340f80..c32ff7c461 100644
--- a/p2p/net/nat/internal/nat/nat.go
+++ b/p2p/net/nat/internal/nat/nat.go
@@ -55,46 +55,42 @@ type NAT interface {
 
 // discoverNATs returns all NATs discovered in the network.
 func discoverNATs(ctx context.Context) ([]NAT, []error) {
-	var nats []NAT
-	var errs []error
-
 	type natsAndErrs struct {
 		nats []NAT
 		errs []error
 	}
-	resCh := make(chan natsAndErrs)
+	upnpCh := make(chan natsAndErrs)
+	pmpCh := make(chan natsAndErrs)
 
-	var pendingJobs int
-
-	pendingJobs++
 	go func() {
+		defer close(upnpCh)
+
+		// We do these UPNP queries sequentially because some routers will fail to handle parallel requests.
 		nats, errs := discoverUPNP_IG1(ctx)
-		select {
-		case resCh <- natsAndErrs{nats, errs}:
-		case <-ctx.Done():
-		}
-	}()
 
-	pendingJobs++
-	go func() {
-		nats, errs := discoverUPNP_IG2(ctx)
-		select {
-		case resCh <- natsAndErrs{nats, errs}:
-		case <-ctx.Done():
+		// Do IG2 after IG1 so that its NAT devices will appear as "better" when we
+		// find the best NAT to return below.
+		n, e := discoverUPNP_IG2(ctx)
+		nats = append(nats, n...)
+		errs = append(errs, e...)
+
+		if len(nats) == 0 {
+			// We don't have a NAT. We should try querying all devices over
+			// SSDP to find a InternetGatewayDevice. This shouldn't be necessary for
+			// a well behaved router.
+			n, e = discoverUPNP_GenIGDev(ctx)
+			nats = append(nats, n...)
+			errs = append(errs, e...)
 		}
-	}()
 
-	pendingJobs++
-	go func() {
-		nats, errs := discoverUPNP_GenIGDev(ctx)
 		select {
-		case resCh <- natsAndErrs{nats, errs}:
+		case upnpCh <- natsAndErrs{nats, errs}:
 		case <-ctx.Done():
 		}
 	}()
 
-	pendingJobs++
 	go func() {
+		defer close(pmpCh)
 		nat, err := discoverNATPMP(ctx)
 		var nats []NAT
 		var errs []error
@@ -104,15 +100,22 @@ func discoverNATs(ctx context.Context) ([]NAT, []error) {
 			nats = append(nats, nat)
 		}
 		select {
-		case resCh <- natsAndErrs{nats, errs}:
+		case pmpCh <- natsAndErrs{nats, errs}:
 		case <-ctx.Done():
 		}
 	}()
 
-	for pendingJobs > 0 {
-		pendingJobs--
+	var nats []NAT
+	var errs []error
+
+	for upnpCh != nil && pmpCh != nil {
 		select {
-		case res := <-resCh:
+		case res := <-pmpCh:
+			pmpCh = nil
+			nats = append(nats, res.nats...)
+			errs = append(errs, res.errs...)
+		case res := <-upnpCh:
+			upnpCh = nil
 			nats = append(nats, res.nats...)
 			errs = append(errs, res.errs...)
 		case <-ctx.Done():