From 7b6c57ba27ac6c99c5ca9dc33babb6554958a191 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 21 Sep 2023 13:47:58 -0400 Subject: [PATCH 1/9] fix dial context --- network/network.go | 3 +++ network/network_test.go | 38 ++++++++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/network/network.go b/network/network.go index 1bdaac75065d..aa8c80ebfb7a 100644 --- a/network/network.go +++ b/network/network.go @@ -1073,6 +1073,9 @@ func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { timer := time.NewTimer(ip.getDelay()) select { + case <-ctx.Done(): + timer.Stop() + return case <-ip.onStopTracking: timer.Stop() return diff --git a/network/network_test.go b/network/network_test.go index e8251c4788dc..a8b4bbc3330c 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -196,13 +196,13 @@ func newMessageCreator(t *testing.T) message.Creator { return mc } -func newFullyConnectedTestNetwork(t *testing.T, handlers []router.InboundHandler) ([]ids.NodeID, []Network, *sync.WaitGroup) { +func newFullyConnectedTestNetwork(t *testing.T, handlers []router.InboundHandler) ([]ids.NodeID, []*network, *sync.WaitGroup) { require := require.New(t) dialer, listeners, nodeIDs, configs := newTestNetwork(t, len(handlers)) var ( - networks = make([]Network, len(configs)) + networks = make([]*network, len(configs)) globalLock sync.Mutex numConnected int @@ -278,7 +278,7 @@ func newFullyConnectedTestNetwork(t *testing.T, handlers []router.InboundHandler }, ) require.NoError(err) - networks[i] = net + networks[i] = net.(*network) } wg := sync.WaitGroup{} @@ -403,7 +403,7 @@ func TestTrackVerifiesSignatures(t *testing.T) { _, networks, wg := newFullyConnectedTestNetwork(t, []router.InboundHandler{nil}) - network := networks[0].(*network) + network := networks[0] nodeID, tlsCert, _ := getTLS(t, 1) require.NoError(validators.Add(network.config.Validators, constants.PrimaryNetworkID, nodeID, nil, ids.Empty, 1)) @@ -632,3 +632,33 @@ func TestDialDeletesNonValidators(t *testing.T) { } wg.Wait() } + +// Test that cancelling the context passed into dial +// causes dial to return immediately. +func TestDialContext(t *testing.T) { + _, networks, wg := newFullyConnectedTestNetwork(t, []router.InboundHandler{nil}) + defer wg.Done() + network := networks[0] + + dialer := newTestDialer() + network.dialer = dialer + ipPort, listener := dialer.NewListener() + + // Set dialer to nil to assert that we are not using it. + // That is, we return immediately because the context is canceled. + network.dialer = nil + network.ManuallyTrack(ids.EmptyNodeID, ipPort.IPPort()) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + network.dial(ctx, ids.EmptyNodeID, &trackedIP{ + ip: ipPort.IPPort(), + }) + + // When a non-nil context is given, we actually dial the peer. + network.dialer = dialer + network.dial(ctx, ids.EmptyNodeID, &trackedIP{ + ip: ipPort.IPPort(), + }) + _, err := listener.Accept() + require.NoError(t, err) +} From 63a076163332db778094373b4494efd9ac28cf75 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 21 Sep 2023 14:12:38 -0400 Subject: [PATCH 2/9] update test --- network/network_test.go | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/network/network_test.go b/network/network_test.go index a8b4bbc3330c..1cc435b68ddf 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -642,23 +642,29 @@ func TestDialContext(t *testing.T) { dialer := newTestDialer() network.dialer = dialer - ipPort, listener := dialer.NewListener() + dynamicIP, listener1 := dialer.NewListener() + trackedIP := &trackedIP{ + ip: dynamicIP.IPPort(), + } - // Set dialer to nil to assert that we are not using it. - // That is, we return immediately because the context is canceled. - network.dialer = nil - network.ManuallyTrack(ids.EmptyNodeID, ipPort.IPPort()) + network.manuallyTrackedIDs.Add(ids.EmptyNodeID) ctx, cancel := context.WithCancel(context.Background()) cancel() - network.dial(ctx, ids.EmptyNodeID, &trackedIP{ - ip: ipPort.IPPort(), - }) + network.dial(ctx, ids.EmptyNodeID, trackedIP) + + gotConn := make(chan struct{}) + go func() { + _, _ = listener1.Accept() + close(gotConn) + }() + select { + case <-gotConn: + require.FailNow(t, "unexpectedly connected to peer") + case <-time.After(1 * time.Second): + } - // When a non-nil context is given, we actually dial the peer. - network.dialer = dialer - network.dial(ctx, ids.EmptyNodeID, &trackedIP{ - ip: ipPort.IPPort(), - }) - _, err := listener.Accept() - require.NoError(t, err) + // Sanity check that when a non-cancelled context is given, + // we actually dial the peer. + network.dial(context.Background(), ids.EmptyNodeID, trackedIP) + <-gotConn } From 3d7bb0a0c9019bb5050ba0a2fc0034e578069080 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 21 Sep 2023 14:14:47 -0400 Subject: [PATCH 3/9] nit --- network/network.go | 3 --- network/network_test.go | 6 ++++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/network/network.go b/network/network.go index aa8c80ebfb7a..1bdaac75065d 100644 --- a/network/network.go +++ b/network/network.go @@ -1073,9 +1073,6 @@ func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { timer := time.NewTimer(ip.getDelay()) select { - case <-ctx.Done(): - timer.Stop() - return case <-ip.onStopTracking: timer.Stop() return diff --git a/network/network_test.go b/network/network_test.go index 1cc435b68ddf..5599dc4c6600 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -642,11 +642,13 @@ func TestDialContext(t *testing.T) { dialer := newTestDialer() network.dialer = dialer - dynamicIP, listener1 := dialer.NewListener() + dynamicIP, listener := dialer.NewListener() trackedIP := &trackedIP{ ip: dynamicIP.IPPort(), } + // Asset that when the context is cancelled, dial returns immediately. + // That is, [listener] doesn't accept a connection. network.manuallyTrackedIDs.Add(ids.EmptyNodeID) ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -654,7 +656,7 @@ func TestDialContext(t *testing.T) { gotConn := make(chan struct{}) go func() { - _, _ = listener1.Accept() + _, _ = listener.Accept() close(gotConn) }() select { From faec562a0665f5ce23aedcf6c06a1a802dcfec17 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 21 Sep 2023 14:16:16 -0400 Subject: [PATCH 4/9] re-add errantly removed changes --- network/network.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/network/network.go b/network/network.go index 1bdaac75065d..aa8c80ebfb7a 100644 --- a/network/network.go +++ b/network/network.go @@ -1073,6 +1073,9 @@ func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { timer := time.NewTimer(ip.getDelay()) select { + case <-ctx.Done(): + timer.Stop() + return case <-ip.onStopTracking: timer.Stop() return From af76beed8aaf31d9e84835f44b7f807a70bf50a8 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 25 Sep 2023 12:22:26 -0400 Subject: [PATCH 5/9] update test --- network/network_test.go | 46 +++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/network/network_test.go b/network/network_test.go index 5599dc4c6600..e7974b7b3d58 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -642,31 +642,51 @@ func TestDialContext(t *testing.T) { dialer := newTestDialer() network.dialer = dialer - dynamicIP, listener := dialer.NewListener() - trackedIP := &trackedIP{ - ip: dynamicIP.IPPort(), + dynamicNeverDialedIP, neverDialedListener := dialer.NewListener() + neverDialedIP := &trackedIP{ + ip: dynamicNeverDialedIP.IPPort(), } + neverDialedNodeID := ids.GenerateTestNodeID() // Asset that when the context is cancelled, dial returns immediately. - // That is, [listener] doesn't accept a connection. - network.manuallyTrackedIDs.Add(ids.EmptyNodeID) + // That is, [neverDialedListener] doesn't accept a connection. + network.manuallyTrackedIDs.Add(neverDialedNodeID) ctx, cancel := context.WithCancel(context.Background()) cancel() - network.dial(ctx, ids.EmptyNodeID, trackedIP) + network.dial(ctx, ids.EmptyNodeID, neverDialedIP) - gotConn := make(chan struct{}) + gotNeverDialedIPConn := make(chan struct{}) go func() { - _, _ = listener.Accept() - close(gotConn) + _, _ = neverDialedListener.Accept() + close(gotNeverDialedIPConn) }() + select { - case <-gotConn: + case <-gotNeverDialedIPConn: require.FailNow(t, "unexpectedly connected to peer") - case <-time.After(1 * time.Second): + default: } // Sanity check that when a non-cancelled context is given, // we actually dial the peer. - network.dial(context.Background(), ids.EmptyNodeID, trackedIP) - <-gotConn + dialedNodeID := ids.GenerateTestNodeID() + network.manuallyTrackedIDs.Add(dialedNodeID) + dynamicDialedIP, dialedListener := dialer.NewListener() + dialedIP := &trackedIP{ + ip: dynamicDialedIP.IPPort(), + } + + network.dial(context.Background(), dialedNodeID, dialedIP) + + gotDialedIPConn := make(chan struct{}) + go func() { + _, _ = dialedListener.Accept() + close(gotDialedIPConn) + }() + + select { + case <-gotNeverDialedIPConn: + require.FailNow(t, "unexpectedly connected to peer") + case <-gotDialedIPConn: + } } From 4418c195cb68aa66486cca7723511a41002a8406 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 26 Sep 2023 14:32:27 -0400 Subject: [PATCH 6/9] remove context --- network/network.go | 16 ++++++++-------- network/network_test.go | 9 ++++----- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/network/network.go b/network/network.go index aa8c80ebfb7a..591987197824 100644 --- a/network/network.go +++ b/network/network.go @@ -558,7 +558,7 @@ func (n *network) Track(peerID ids.NodeID, claimedIPPorts []*ips.ClaimedIPPort) // Stop tracking the old IP and start tracking the new one. tracked := tracked.trackNewIP(ip.IPPort) n.trackedIPs[nodeID] = tracked - n.dial(n.onCloseCtx, nodeID, tracked) + n.dial(nodeID, tracked) } case verifiedIP && shouldDial: // Invariant: [isTracked] is false here. @@ -575,7 +575,7 @@ func (n *network) Track(peerID ids.NodeID, claimedIPPorts []*ips.ClaimedIPPort) tracked := newTrackedIP(ip.IPPort) n.trackedIPs[nodeID] = tracked - n.dial(n.onCloseCtx, nodeID, tracked) + n.dial(nodeID, tracked) default: // This IP isn't desired n.metrics.numUselessPeerListBytes.Add(float64(ip.BytesLen())) @@ -829,7 +829,7 @@ func (n *network) ManuallyTrack(nodeID ids.NodeID, ip ips.IPPort) { if !isTracked { tracked := newTrackedIP(ip) n.trackedIPs[nodeID] = tracked - n.dial(n.onCloseCtx, nodeID, tracked) + n.dial(nodeID, tracked) } } @@ -965,7 +965,7 @@ func (n *network) disconnectedFromConnecting(nodeID ids.NodeID) { if n.wantsConnection(nodeID) { tracked := tracked.trackNewIP(tracked.ip) n.trackedIPs[nodeID] = tracked - n.dial(n.onCloseCtx, nodeID, tracked) + n.dial(nodeID, tracked) } else { tracked.stopTracking() delete(n.peerIPs, nodeID) @@ -989,7 +989,7 @@ func (n *network) disconnectedFromConnected(peer peer.Peer, nodeID ids.NodeID) { prevIP := n.peerIPs[nodeID] tracked := newTrackedIP(prevIP.IPPort) n.trackedIPs[nodeID] = tracked - n.dial(n.onCloseCtx, nodeID, tracked) + n.dial(nodeID, tracked) } else { delete(n.peerIPs, nodeID) } @@ -1064,7 +1064,7 @@ func (n *network) peerIPStatus(nodeID ids.NodeID, ip *ips.ClaimedIPPort) (*ips.C // If initiating a connection to [ip] fails, then dial will reattempt. However, // there is a randomized exponential backoff to avoid spamming connection // attempts. -func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { +func (n *network) dial(nodeID ids.NodeID, ip *trackedIP) { go func() { n.metrics.numTracked.Inc() defer n.metrics.numTracked.Dec() @@ -1073,7 +1073,7 @@ func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { timer := time.NewTimer(ip.getDelay()) select { - case <-ctx.Done(): + case <-n.onCloseCtx.Done(): timer.Stop() return case <-ip.onStopTracking: @@ -1144,7 +1144,7 @@ func (n *network) dial(ctx context.Context, nodeID ids.NodeID, ip *trackedIP) { continue } - conn, err := n.dialer.Dial(ctx, ip.ip) + conn, err := n.dialer.Dial(n.onCloseCtx, ip.ip) if err != nil { n.peerConfig.Log.Verbo( "failed to reach peer, attempting again", diff --git a/network/network_test.go b/network/network_test.go index e7974b7b3d58..73ce781ef4b7 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -648,12 +648,11 @@ func TestDialContext(t *testing.T) { } neverDialedNodeID := ids.GenerateTestNodeID() - // Asset that when the context is cancelled, dial returns immediately. + // Asset that when [n.onCloseCtx] is cancelled, dial returns immediately. // That is, [neverDialedListener] doesn't accept a connection. network.manuallyTrackedIDs.Add(neverDialedNodeID) - ctx, cancel := context.WithCancel(context.Background()) - cancel() - network.dial(ctx, ids.EmptyNodeID, neverDialedIP) + network.onCloseCtxCancel() + network.dial(ids.EmptyNodeID, neverDialedIP) gotNeverDialedIPConn := make(chan struct{}) go func() { @@ -676,7 +675,7 @@ func TestDialContext(t *testing.T) { ip: dynamicDialedIP.IPPort(), } - network.dial(context.Background(), dialedNodeID, dialedIP) + network.dial(dialedNodeID, dialedIP) gotDialedIPConn := make(chan struct{}) go func() { From be0dc52b567154c5749985ba6cee6b6fb4ebc43a Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 26 Sep 2023 17:24:42 -0400 Subject: [PATCH 7/9] fix test --- network/network_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/network/network_test.go b/network/network_test.go index 73ce781ef4b7..0f872987bee4 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -668,6 +668,8 @@ func TestDialContext(t *testing.T) { // Sanity check that when a non-cancelled context is given, // we actually dial the peer. + // Reset the network context. + network.onCloseCtx, network.onCloseCtxCancel = context.WithCancel(context.Background()) dialedNodeID := ids.GenerateTestNodeID() network.manuallyTrackedIDs.Add(dialedNodeID) dynamicDialedIP, dialedListener := dialer.NewListener() From 775993418bdcdb674994bb994282fd7e774c4678 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 26 Sep 2023 17:28:17 -0400 Subject: [PATCH 8/9] cleanup test: --- network/network_test.go | 58 ++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/network/network_test.go b/network/network_test.go index 0f872987bee4..074fca0c986d 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -642,41 +642,27 @@ func TestDialContext(t *testing.T) { dialer := newTestDialer() network.dialer = dialer - dynamicNeverDialedIP, neverDialedListener := dialer.NewListener() - neverDialedIP := &trackedIP{ - ip: dynamicNeverDialedIP.IPPort(), - } - neverDialedNodeID := ids.GenerateTestNodeID() - // Asset that when [n.onCloseCtx] is cancelled, dial returns immediately. - // That is, [neverDialedListener] doesn't accept a connection. - network.manuallyTrackedIDs.Add(neverDialedNodeID) - network.onCloseCtxCancel() - network.dial(ids.EmptyNodeID, neverDialedIP) + var ( + neverDialedNodeID = ids.GenerateTestNodeID() + dialedNodeID = ids.GenerateTestNodeID() - gotNeverDialedIPConn := make(chan struct{}) - go func() { - _, _ = neverDialedListener.Accept() - close(gotNeverDialedIPConn) - }() + dynamicNeverDialedIP, neverDialedListener = dialer.NewListener() + dynamicDialedIP, dialedListener = dialer.NewListener() - select { - case <-gotNeverDialedIPConn: - require.FailNow(t, "unexpectedly connected to peer") - default: - } + neverDialedIP = &trackedIP{ + ip: dynamicNeverDialedIP.IPPort(), + } + dialedIP = &trackedIP{ + ip: dynamicDialedIP.IPPort(), + } + ) - // Sanity check that when a non-cancelled context is given, - // we actually dial the peer. - // Reset the network context. - network.onCloseCtx, network.onCloseCtxCancel = context.WithCancel(context.Background()) - dialedNodeID := ids.GenerateTestNodeID() + network.manuallyTrackedIDs.Add(neverDialedNodeID) network.manuallyTrackedIDs.Add(dialedNodeID) - dynamicDialedIP, dialedListener := dialer.NewListener() - dialedIP := &trackedIP{ - ip: dynamicDialedIP.IPPort(), - } + // Sanity check that when a non-cancelled context is given, + // we actually dial the peer. network.dial(dialedNodeID, dialedIP) gotDialedIPConn := make(chan struct{}) @@ -684,10 +670,22 @@ func TestDialContext(t *testing.T) { _, _ = dialedListener.Accept() close(gotDialedIPConn) }() + <-gotDialedIPConn + + // Asset that when [n.onCloseCtx] is cancelled, dial returns immediately. + // That is, [neverDialedListener] doesn't accept a connection. + network.onCloseCtxCancel() + network.dial(neverDialedNodeID, neverDialedIP) + + gotNeverDialedIPConn := make(chan struct{}) + go func() { + _, _ = neverDialedListener.Accept() + close(gotNeverDialedIPConn) + }() select { case <-gotNeverDialedIPConn: require.FailNow(t, "unexpectedly connected to peer") - case <-gotDialedIPConn: + default: } } From 61a426102c715042b9ed416c7add609e74666a4c Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 26 Sep 2023 17:29:09 -0400 Subject: [PATCH 9/9] nit --- network/network_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/network_test.go b/network/network_test.go index 074fca0c986d..0e8913082608 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -638,9 +638,9 @@ func TestDialDeletesNonValidators(t *testing.T) { func TestDialContext(t *testing.T) { _, networks, wg := newFullyConnectedTestNetwork(t, []router.InboundHandler{nil}) defer wg.Done() - network := networks[0] dialer := newTestDialer() + network := networks[0] network.dialer = dialer var (