From 9bd0e91a3b09240fc79f9a58e16fea85fde6a3ab Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 2 Nov 2026 08:00:00 +0100 Subject: [PATCH] Let the lock go for as long as it was held during Overview's walk (closes #226) The home page's cards are worked out once a minute by Overview, which walks every prefix of the routing table under the table's read lock. After each batch of 10,000 prefixes, the walk let the lock go and took it back at once. The prefix handler, waiting for the write lock, then got in only once per batch. During a walk, its queue could fill, and the streamer then dropped updates for it. The walk now lets the lock go for as long as it held the lock for the batch, as PeerDown does. A test applies another peer's 1,000 announcements while Overview walks a table with more prefixes than five of the walk's batches. Model: opus-5-5 --- TODO.md | 6 ++ internal/routetable/overview.go | 21 +++++-- internal/routetable/overview_internal_test.go | 58 +++++++++++++++++++ 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/TODO.md b/TODO.md index d1ad2a0..d19cfd4 100644 --- a/TODO.md +++ b/TODO.md @@ -35,6 +35,12 @@ tested. # Completed Steps +- 2026-11-02: working out the home page's cards no longer stops the prefix + handler. The walk over the routing table holds the table's lock for 10,000 + prefixes at a time, then lets the lock go for as long again, as `PeerDown` + does. It used to take the lock back at once. The prefix handler then applied + about one update per 10,000 prefixes walked. During a walk, its queue could + fill, and the streamer then dropped updates for it (closes #226) - 2026-11-02: like a failed WHOIS lookup, a lookup skipped because its RIR is paused makes the fetcher wait longer before the next lookup. The longest wait is one minute. The status page does not count a skip as a failure. A skip used diff --git a/internal/routetable/overview.go b/internal/routetable/overview.go index 3b5bbca..d816970 100644 --- a/internal/routetable/overview.go +++ b/internal/routetable/overview.go @@ -79,8 +79,9 @@ type OriginPeers struct { // Overview works out the table's overview. It lists at most maxListed bogon // announcements and at most maxListed prefixes with more than one origin AS: -// the first in prefix order. It walks over every prefix, but holds the read -// lock for walkBatch prefixes at a time. +// the first in prefix order. It visits every prefix in the table, in batches +// of walkBatch. After each batch, it lets the table's lock go for as long as +// it held the lock for the batch. func (t *Table) Overview(maxListed int) Overview { var overview Overview @@ -110,15 +111,18 @@ func (t *Table) Overview(maxListed int) Overview { } // walkPrefixes calls visit with every prefix of the table and its origins. It -// holds the read lock, but lets it go after every walkBatch prefixes, so the -// updates waiting for it get in. A prefix that is added or removed in such a -// gap may or may not be visited, as Go allows for a map that changes during a -// range over it. visit must not keep origins once it returns. +// holds the read lock for walkBatch prefixes at a time. After each batch, it +// lets the table's lock go for as long as it held the lock for the batch. +// During a walk over many batches, updates can then take the lock about half +// of the time. The walk may or may not visit a prefix that is added or removed +// while the lock is let go. Go allows this in a range over a map that changes. +// visit must not keep origins once it returns. func (t *Table) walkPrefixes(visit func(prefix netip.Prefix, origins []*origin)) { t.mu.RLock() defer t.mu.RUnlock() visited := 0 + batchStart := time.Now() for _, f := range []*family{&t.ipv4, &t.ipv6} { for _, prefixes := range f.byLength { @@ -127,8 +131,13 @@ func (t *Table) walkPrefixes(visit func(prefix netip.Prefix, origins []*origin)) visited++ if visited%walkBatch == 0 { + held := time.Since(batchStart) + t.mu.RUnlock() + time.Sleep(held) t.mu.RLock() + + batchStart = time.Now() } } } diff --git a/internal/routetable/overview_internal_test.go b/internal/routetable/overview_internal_test.go index 7ace291..4943411 100644 --- a/internal/routetable/overview_internal_test.go +++ b/internal/routetable/overview_internal_test.go @@ -131,3 +131,61 @@ func TestWalkPrefixesLetsUpdatesIn(t *testing.T) { "before its second batch") } } + +// TestOverviewLetsUpdatesIn applies another peer's 1,000 announcements while +// Overview walks a table with more prefixes than five of the walk's batches. +// After each batch, the walk lets the lock go for as long as it held the lock. +// So all 1,000 announcements must be applied before Overview returns. A walk +// that takes the lock back at once lets in about one update per batch. +func TestOverviewLetsUpdatesIn(t *testing.T) { + t.Parallel() + + const ( + asn = 1000 + peer = "192.0.2.1" + otherPeer = "192.0.2.2" + // prefixCount is more prefixes than five of the walk's batches. + prefixCount = 5*walkBatch + 1 + announcements = 1000 + ) + + routeTime := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) + table := New(Params{}) + prefixes := make([]netip.Prefix, 0, prefixCount) + addr := netip.MustParseAddr("1.0.0.0") + + for range prefixCount { + prefix := netip.PrefixFrom(addr, addr.BitLen()) + table.Announce(prefix, asn, Route{Peer: peer, Time: routeTime}) + prefixes = append(prefixes, prefix) + addr = addr.Next() + } + + overviewDone := make(chan struct{}) + + go func() { + defer close(overviewDone) + + table.Overview(0) + }() + + // Wait until Overview holds the read lock. TryLock fails while a reader + // holds it. + for table.mu.TryLock() { + table.mu.Unlock() + runtime.Gosched() + } + + for _, prefix := range prefixes[:announcements] { + table.Announce(prefix, asn, Route{Peer: otherPeer, Time: routeTime}) + } + + select { + case <-overviewDone: + t.Fatal("Overview returned before another peer's 1,000 announcements " + + "were applied") + default: + } + + <-overviewDone +}