diff --git a/docs/hardening-backlog.md b/docs/hardening-backlog.md index eb8a117..62e7423 100644 --- a/docs/hardening-backlog.md +++ b/docs/hardening-backlog.md @@ -61,7 +61,7 @@ All items below survived an independent skeptic re-reading the cited code. | 31 | ⚪ low | small | operational | `report-falsely-reassuring` | open | | 32 | ⚪ low | small | data-integrity | `cas-no-cid-content-verification` | open | | 33 | ⚪ low | trivial | test-gap | `merkle-no-segwit-witness-block` | open | -| 34 | ⚪ low | trivial | reliability | `p2p-stop-not-idempotent` | open | +| 34 | ⚪ low | trivial | reliability | `p2p-stop-not-idempotent` | ✅ done (#105) | | 35 | ⚪ low | small | test-gap | `walletstore-no-differential-fuzz` | open | | 36 | ⚪ low | small | correctness | `headers-no-future-time-and-false-ban` | open | | 37 | ⚪ low | small | test-gap | `cmd-ion-node-zero-tests` | open | diff --git a/internal/p2p/client_test.go b/internal/p2p/client_test.go index a4760eb..b1b2b5d 100644 --- a/internal/p2p/client_test.go +++ b/internal/p2p/client_test.go @@ -132,3 +132,19 @@ func TestPeerConfigBasics(t *testing.T) { t.Errorf("NewestBlock() = %v,%d,%v; want genesis hash, 0, nil", h, height, err) } } + +// TestStopIsIdempotent: a double Stop (defer + explicit, or concurrent shutdown +// paths) must not panic on a closed quit channel. +func TestStopIsIdempotent(t *testing.T) { + cfg, err := config.New("simnet") + if err != nil { + t.Fatalf("config: %v", err) + } + cl := New(cfg, chain.NewHeaderChain(cfg.Params), log.Noop()) + if err := cl.Stop(); err != nil { + t.Fatalf("first Stop: %v", err) + } + if err := cl.Stop(); err != nil { // must not panic (close of closed channel) + t.Fatalf("second Stop: %v", err) + } +} diff --git a/internal/p2p/peer.go b/internal/p2p/peer.go index 3f4e79f..08b4d8b 100644 --- a/internal/p2p/peer.go +++ b/internal/p2p/peer.go @@ -49,6 +49,7 @@ type Client struct { syncedCh chan struct{} syncOnce sync.Once quit chan struct{} + stopOnce sync.Once // allowSelfConns relaxes btcd's self-connection (nonce) guard. It is only // set in tests that run two peers in the same process (which share btcd's @@ -282,11 +283,15 @@ func (c *Client) Start() error { } // Stop tears down peering. +// Stop is idempotent: a double Stop (e.g. a defer plus an explicit shutdown, or two +// concurrent shutdown paths) must not panic on a closed quit channel. func (c *Client) Stop() error { - close(c.quit) - if c.cm != nil { - c.cm.Stop() - } + c.stopOnce.Do(func() { + close(c.quit) + if c.cm != nil { + c.cm.Stop() + } + }) return nil }