From feecdb0ca32d71fcccce93beaeff80d2fd643e8e Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 2 Nov 2026 05:51:12 +0000 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 its read lock. After each batch of 10,000 prefixes it 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. Its queue filled, and the streamer dropped updates for it. The walk now lets the lock go for as long as it held it for the batch, as PeerDown does. A test applies another peer's 1,000 announcements while Overview walks a table of more than five 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..1581f7f 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. Its queue filled, and the + streamer dropped updates for it once a minute (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..b872fb2 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. A prefix that is added or removed while the lock is let go 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. 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..38d510d 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 of more than five batches. After each batch, the walk +// lets the lock go for as long as it held it, so all of them 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 +}