Skip to content

Commit f2c9d10

Browse files
committed
fix(eth): make peer close idempotent
Guard the close of term with a sync.Once so repeated or concurrent calls cannot panic on a double close. Ownership of term stays with peerSet.Unregister; the once hardens the exactly-once invariant.
1 parent 40a0fa9 commit f2c9d10

2 files changed

Lines changed: 39 additions & 4 deletions

File tree

eth/peer.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ type peer struct {
109109

110110
term chan struct{} // Termination channel to stop the broadcaster
111111
broadcastWg sync.WaitGroup // Tracks the broadcaster goroutines so they can be awaited
112+
closeOnce sync.Once // Ensures term is closed exactly once
112113

113114
knownVote mapset.Set[common.Hash] // Set of BFT Vote known to be known by this peer
114115
knownTimeout mapset.Set[common.Hash] // Set of BFT timeout known to be known by this peer
@@ -306,9 +307,10 @@ func (p *peer) announceTransactions() {
306307
}
307308
}
308309

309-
// close signals the broadcast goroutine to terminate.
310+
// close signals the broadcast goroutine to terminate. It is safe for
311+
// concurrent and repeated calls: only the first call closes term.
310312
func (p *peer) close() {
311-
close(p.term)
313+
p.closeOnce.Do(func() { close(p.term) })
312314
}
313315

314316
// Info gathers and returns a collection of metadata known about a peer.

eth/peer_test.go

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,40 @@ func waitBroadcasters(t *testing.T, wg *sync.WaitGroup) {
8585
}
8686
}
8787

88+
// TestPeerCloseIsIdempotent verifies that close can be called repeatedly and
89+
// from concurrent goroutines without panicking on a double close of term. The
90+
// peer set owns term via Unregister, but making close idempotent removes the
91+
// risk of a "close of closed channel" panic from future callers.
92+
func TestPeerCloseIsIdempotent(t *testing.T) {
93+
app, net := p2p.MsgPipe()
94+
defer app.Close()
95+
var id enode.ID
96+
if _, err := rand.Read(id[:]); err != nil {
97+
t.Fatalf("failed to generate random peer id: %v", err)
98+
}
99+
p := newPeer(xdc100, p2p.NewPeer(id, "close", nil), net, func(common.Hash) *types.Transaction { return nil })
100+
101+
var wg sync.WaitGroup
102+
for i := 0; i < 8; i++ {
103+
wg.Add(1)
104+
go func() {
105+
defer wg.Done()
106+
p.close()
107+
}()
108+
}
109+
p.close()
110+
wg.Wait()
111+
112+
// All concurrent calls returned without panic, so idempotency holds. As a
113+
// final sanity check, the first call must have closed term; the default
114+
// case fails fast if close ever stops closing it.
115+
select {
116+
case <-p.term:
117+
default:
118+
t.Fatal("close did not terminate the peer")
119+
}
120+
}
121+
88122
// countingMsgWriter wraps a p2p.MsgReadWriter and counts every write attempt,
89123
// allowing tests to assert that a peer stops sending after a network error.
90124
type countingMsgWriter struct {
@@ -118,8 +152,7 @@ func newBroadcastTestPeer(t *testing.T, name string) (*peer, *types.Transaction,
118152
}
119153
counter := &countingMsgWriter{MsgReadWriter: net}
120154
p := newPeer(xdc100, p2p.NewPeer(id, name, nil), counter, func(common.Hash) *types.Transaction { return tx })
121-
var closeOnce sync.Once
122-
terminate := func() { closeOnce.Do(p.close) }
155+
terminate := p.close
123156
t.Cleanup(terminate)
124157
app.Close()
125158
return p, tx, counter, terminate

0 commit comments

Comments
 (0)