From 87a5f8ac264c122792318e6139094ed32cbe7e7a Mon Sep 17 00:00:00 2001 From: Lexer747 Date: Fri, 21 Aug 2026 17:39:04 +0100 Subject: [PATCH] improve locking API --- cmd/subcommands/acci-ping/application.go | 2 +- files/files.go | 2 +- graph/data/serialisation_property_test.go | 6 +-- graph/drawing.go | 20 ++++---- graph/export_test.go | 10 ++-- graph/graphdata/graphdata.go | 49 ++++++++++-------- graph/graphdata/graphdata_test.go | 28 +++++++---- tools/verify.sh | 2 +- utils/syncutils/syncutils.go | 60 +++++++++++++++++++++++ 9 files changed, 128 insertions(+), 51 deletions(-) create mode 100644 utils/syncutils/syncutils.go diff --git a/cmd/subcommands/acci-ping/application.go b/cmd/subcommands/acci-ping/application.go index 1d396e8..7f154c8 100644 --- a/cmd/subcommands/acci-ping/application.go +++ b/cmd/subcommands/acci-ping/application.go @@ -1,6 +1,6 @@ // Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. // -// Copyright 2024-2025 Lexer747 +// Copyright 2024-2026 Lexer747 // // SPDX-License-Identifier: GPL-2.0-only diff --git a/files/files.go b/files/files.go index dbcefc4..8ad5dde 100644 --- a/files/files.go +++ b/files/files.go @@ -1,6 +1,6 @@ // Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. // -// Copyright 2024-2025 Lexer747 +// Copyright 2024-2026 Lexer747 // // SPDX-License-Identifier: GPL-2.0-only diff --git a/graph/data/serialisation_property_test.go b/graph/data/serialisation_property_test.go index f656dfd..2bc1587 100644 --- a/graph/data/serialisation_property_test.go +++ b/graph/data/serialisation_property_test.go @@ -1,6 +1,6 @@ // Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. // -// Copyright 2025 Lexer747 +// Copyright 2025-2026 Lexer747 // // SPDX-License-Identifier: GPL-2.0-only @@ -19,8 +19,8 @@ import ( ) const ( - maxSliceSize = 1024 << 1 - maxStatsSize = 1024 << 3 + maxSliceSize = 512 << 1 + maxStatsSize = 512 << 3 ) func TestCompactTimeSpan_Property(t *testing.T) { diff --git a/graph/drawing.go b/graph/drawing.go index 97f2f95..1a5b113 100644 --- a/graph/drawing.go +++ b/graph/drawing.go @@ -47,8 +47,8 @@ func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { // size of the terminal. Side note - we deliberately don't attach this to the terminal size channel since // it's locked to the targeted FPS of this frame time anyway so just adds extra work. s := g.Term.GetSize() - g.data.Lock() - count := g.data.LockFreeTotalCount() + lg := g.data.Lock() + count := g.data.LockFreeTotalCount(lg) spinnerValue := "" if cfg.drawSpinner { spinnerValue = g.lastFrame.spinnerData.spinner(s) @@ -56,7 +56,7 @@ func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { g.drawingBuffer.Get(draw.SpinnerIndex).WriteString(spinnerValue) } if count == g.lastFrame.PacketCount && g.lastFrame.Match(s, cfg) { - g.data.Unlock() // fast path the frame didn't change + g.data.Unlock(lg) // fast path the frame didn't change if updateGui := g.checkGUI(); updateGui != nil { return updateGui } @@ -67,20 +67,20 @@ func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { } if count == 0 { // nothing to do - g.data.Unlock() + g.data.Unlock(lg) return noFrame } g.drawingBuffer.Reset(draw.GraphIndexes...) - header := g.data.LockFreeHeader() - iter := g.data.LockFreeIter(cfg.followLatestSpan) + header := g.data.LockFreeHeader(lg) + iter := g.data.LockFreeIter(lg, cfg.followLatestSpan) x := computeXAxis( g.drawingBuffer.Get(draw.XAxisIndex), g.drawingBuffer.Get(draw.BarIndex), s, header.TimeSpan, - g.data.LockFreeSpanInfos(), + g.data.LockFreeSpanInfos(lg), cfg.followLatestSpan, int(iter.Total), ) @@ -88,7 +88,7 @@ func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { if cfg.followLatestSpan { yStats = x.spans[0].pingStats } - y := computeYAxis(g.drawingBuffer.Get(draw.YAxisIndex), s, yStats, g.data.LockFreeURL(), cfg.yAxisScale) + y := computeYAxis(g.drawingBuffer.Get(draw.YAxisIndex), s, yStats, g.data.LockFreeURL(lg), cfg.yAxisScale) computeFrame( g, g.drawingBuffer.Get(draw.GradientIndex), @@ -96,14 +96,14 @@ func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { g.drawingBuffer.Get(draw.DroppedIndex), g.drawingBuffer.Get(draw.KeyIndex), iter, - g.data.LockFreeRuns(), + g.data.LockFreeRuns(lg), x, y, s, ) g.drawingBuffer.Get(draw.SpinnerIndex).WriteString(spinnerValue) // Everything we need is now cached we can unlock a bit early while we tidy up for the next frame paintFrame := withGUI(g.drawingBuffer) noGUI := withoutGUI(g.drawingBuffer) - g.data.Unlock() + g.data.Unlock(lg) g.lastFrame = frame{ PacketCount: count, yAxis: y, diff --git a/graph/export_test.go b/graph/export_test.go index 81895ab..1dbe3eb 100644 --- a/graph/export_test.go +++ b/graph/export_test.go @@ -45,16 +45,16 @@ type XAxisSpanBounds struct { // ComputeXAxisBounds runs the internal x-axis layout and returns the per-span pixel bounds plus the axis size, // letting tests assert layout invariants (e.g. the drawable area is fully used) without golden files. func (g *Graph) ComputeXAxisBounds(s terminal.Size, following bool) []XAxisSpanBounds { - g.data.Lock() - defer g.data.Unlock() - header := g.data.LockFreeHeader() - iter := g.data.LockFreeIter(following) + lf := g.data.Lock() + defer g.data.Unlock(lf) + header := g.data.LockFreeHeader(lf) + iter := g.data.LockFreeIter(lf, following) x := computeXAxis( bytes.NewConcurrentBuf(), bytes.NewConcurrentBuf(), s, header.TimeSpan, - g.data.LockFreeSpanInfos(), + g.data.LockFreeSpanInfos(lf), following, int(iter.Total), ) diff --git a/graph/graphdata/graphdata.go b/graph/graphdata/graphdata.go index 842e1af..edbd35c 100644 --- a/graph/graphdata/graphdata.go +++ b/graph/graphdata/graphdata.go @@ -1,6 +1,6 @@ // Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. // -// Copyright 2024-2025 Lexer747 +// Copyright 2024-2026 Lexer747 // // SPDX-License-Identifier: GPL-2.0-only @@ -13,6 +13,7 @@ import ( "github.com/Lexer747/acci-ping/graph/data" "github.com/Lexer747/acci-ping/ping" + "github.com/Lexer747/acci-ping/utils/syncutils" ) // NOTE: GraphData does not have a [data.FromCompact] implementation because it is meant to be less strict layer on-top @@ -42,48 +43,56 @@ func NewGraphData(d *data.Data) *GraphData { } func (gd *GraphData) AddPoint(p ping.PingResults) { - gd.Lock() - defer gd.Unlock() + gd.m.Lock() + defer gd.m.Unlock() gd.data.AddPoint(p) gd.addPointToSpans(p.Data, gd.data.TotalCount-1) } func (gd *GraphData) TotalCount() int64 { - gd.Lock() - defer gd.Unlock() + gd.m.Lock() + defer gd.m.Unlock() return gd.data.TotalCount } func (gd *GraphData) String() string { - gd.Lock() - defer gd.Unlock() + gd.m.Lock() + defer gd.m.Unlock() return gd.data.String() } func (gd *GraphData) Summary() string { - gd.Lock() - defer gd.Unlock() + gd.m.Lock() + defer gd.m.Unlock() return gd.data.Summary() } -func (gd *GraphData) Lock() { +// This a compile time proof that the lock was actually acquired from this package, it does not guarantee if +// multiple locks are used that the correct one is given and there are still workarounds if malice is desired, +// but this is more a nice hint for the callees of the lock free API to only lock once and pass the proof +// around. +type graphDataLock struct{} +type LockFree *syncutils.LockGuard[graphDataLock] + +func (gd *GraphData) Lock() LockFree { gd.m.Lock() + return syncutils.New[graphDataLock](gd.m) } -func (gd *GraphData) Unlock() { +func (gd *GraphData) Unlock(proof LockFree) { gd.m.Unlock() } -func (gd *GraphData) LockFreeTotalCount() int64 { return gd.data.TotalCount } -func (gd *GraphData) LockFreeHeader() *data.Header { return gd.data.Header } -func (gd *GraphData) LockFreeURL() string { return gd.data.URL } -func (gd *GraphData) LockFreeRuns() *data.Runs { return gd.data.Runs } -func (gd *GraphData) LockFreeSpanInfos() Spans { return gd.spans } +func (gd *GraphData) LockFreeTotalCount(proof LockFree) int64 { return gd.data.TotalCount } +func (gd *GraphData) LockFreeHeader(proof LockFree) *data.Header { return gd.data.Header } +func (gd *GraphData) LockFreeURL(proof LockFree) string { return gd.data.URL } +func (gd *GraphData) LockFreeRuns(proof LockFree) *data.Runs { return gd.data.Runs } +func (gd *GraphData) LockFreeSpanInfos(proof LockFree) Spans { return gd.spans } -func (gd *GraphData) LockFreeIter(followLatestSpan bool) *Iter { +func (gd *GraphData) LockFreeIter(proof LockFree, followLatestSpan bool) *Iter { offset := int64(0) - total := gd.LockFreeTotalCount() + total := gd.LockFreeTotalCount(proof) if followLatestSpan { - spans := gd.LockFreeSpanInfos() + spans := gd.LockFreeSpanInfos(proof) lastIndex := len(spans) - 1 spansExceptLast := spans[:lastIndex] offset = int64(spansExceptLast.Count()) @@ -93,7 +102,7 @@ func (gd *GraphData) LockFreeIter(followLatestSpan bool) *Iter { return &Iter{ Total: total, d: gd.data, - spans: gd.LockFreeSpanInfos(), + spans: gd.LockFreeSpanInfos(proof), offset: offset, } } diff --git a/graph/graphdata/graphdata_test.go b/graph/graphdata/graphdata_test.go index d504eba..e20d595 100644 --- a/graph/graphdata/graphdata_test.go +++ b/graph/graphdata/graphdata_test.go @@ -1,6 +1,6 @@ // Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. // -// Copyright 2024-2025 Lexer747 +// Copyright 2024-2026 Lexer747 // // SPDX-License-Identifier: GPL-2.0-only @@ -166,14 +166,16 @@ func (test BasicTimeSpanTest) Run(t *testing.T) { for _, point := range test.Points { gd.AddPoint(ping.PingResults{Data: point}) } - assert.Assert(t, is.Len(gd.LockFreeSpanInfos(), test.ExpectedSpanCount)) + lf := gd.Lock() + defer gd.Unlock(lf) + assert.Assert(t, is.Len(gd.LockFreeSpanInfos(lf), test.ExpectedSpanCount)) - assertEveryPointHasSpan(t, gd, gd.LockFreeSpanInfos()) + assertEveryPointHasSpan(t, gd, lf, gd.LockFreeSpanInfos(lf)) } -func assertEveryPointHasSpan(t *testing.T, gd *graphdata.GraphData, actual []*graphdata.SpanInfo) { +func assertEveryPointHasSpan(t *testing.T, gd *graphdata.GraphData, lf graphdata.LockFree, actual []*graphdata.SpanInfo) { t.Helper() - iter := gd.LockFreeIter(false) + iter := gd.LockFreeIter(lf, false) for i := range iter.Total { p := iter.Get(i) timestamp := p.Timestamp @@ -207,16 +209,20 @@ func (test TimeSpanTest) Run(t *testing.T) { gd.AddPoint(ping.PingResults{Data: point}) index++ } - actual := gd.LockFreeSpanInfos() + lf := gd.Lock() + actual := gd.LockFreeSpanInfos(lf) + gd.Unlock(lf) assert.Check(t, is.DeepEqual(graphdata.Spans(expectedSpans), actual, utils_th.AllowAllUnexported), "index %d | %+v", i, span) } - actual := gd.LockFreeSpanInfos() + lf := gd.Lock() + defer gd.Unlock(lf) + actual := gd.LockFreeSpanInfos(lf) assert.Assert(t, is.Len(actual, len(expectedSpans))) for i := range actual { assert.Check(t, is.DeepEqual(expectedSpans[i], actual[i], utils_th.AllowAllUnexported), "index %d", i) } - assertEveryPointHasSpan(t, gd, actual) + assertEveryPointHasSpan(t, gd, lf, actual) } type TimeSpanFileTest struct { @@ -230,7 +236,9 @@ func (test TimeSpanFileTest) Run(t *testing.T) { t.Parallel() d := th.GetFromFile(t, test.File) gd := graphdata.NewGraphData(d) - actualSpans := gd.LockFreeSpanInfos() + lf := gd.Lock() + defer gd.Unlock(lf) + actualSpans := gd.LockFreeSpanInfos(lf) assert.Assert(t, is.Len(actualSpans, test.ExpectedSpanCount)) if len(test.ExpectedSpans) != 0 { actual := sliceutils.Map(actualSpans, func(si *graphdata.SpanInfo) *data.TimeSpan { return si.TimeSpan }) @@ -238,5 +246,5 @@ func (test TimeSpanFileTest) Run(t *testing.T) { assert.Assert(t, is.DeepEqual(span, actual[i]), "index %d", i) } } - assertEveryPointHasSpan(t, gd, gd.LockFreeSpanInfos()) + assertEveryPointHasSpan(t, gd, lf, gd.LockFreeSpanInfos(lf)) } diff --git a/tools/verify.sh b/tools/verify.sh index ff02728..24d9031 100755 --- a/tools/verify.sh +++ b/tools/verify.sh @@ -8,7 +8,7 @@ go mod tidy goimports -w . -golangci-lint run +golangci-lint run --max-same-issues 0 --max-issues-per-linter 0 ROOT=$(git rev-parse --show-toplevel) diff --git a/utils/syncutils/syncutils.go b/utils/syncutils/syncutils.go new file mode 100644 index 0000000..cb56a5e --- /dev/null +++ b/utils/syncutils/syncutils.go @@ -0,0 +1,60 @@ +// Use of this source code is governed by a GPL-2 license that can be found in the LICENSE file. +// +// Copyright 2026 Lexer747 +// +// SPDX-License-Identifier: GPL-2.0-only + +package syncutils + +import "sync" + +// LockGuard provides a proof of ownership model for locks from [sync], a lock free API may be written such +// that each function in said API requires that a [LockGuard] is also given. This is why the type arg exists +// merely to be a compile time opt-in for a package specific guard. Runtime checks are opt-in and would +// prevent lock guard sharing from multiple lock guards from the same package. +// +// Example usage: +// +// type myPackageLock struct{} // private to the package +// type LockFree *syncutils.LockGuard[myPackageLock] // Alias for convenience +// +// // normally dangerous lock free interface now hands out a this proof instead of void. +// func (foo *Bar) Lock() LockFree { +// foo.m.Lock() +// return syncutils.New[myPackageLock](foo.m) +// } +// func (foo *Bar) Unlock(proof LockFree) { foo.m.Unlock() } +// // The safe version for use when perf isn't a concern +// func (foo *Bar) SafeAlgo() { +// foo.m.Lock() +// defer foo.m.Unlock() +// foo.hardAlgo() +// } +// // Normally a flight risk as someone could call this without the lock, but since they +// // have to pass proof we can verify if it's lock (and handle that accordingly) +// func (foo *Bar) LockFreeHardAlgo(proof LockFree) { +// if proof.IsMine(foo.M) { +// foo.hardAlgo() +// return +// } +// panic("wrong proof of locking!") +// } +type LockGuard[T any] struct { + l sync.Locker +} + +// New constructs a new lock guard on the given locker, no operations are performed on the lock its merely +// held so that [IsMine] can confirm that this is in fact the same lock that was reference. +func New[T any](locker sync.Locker) *LockGuard[T] { + return &LockGuard[T]{l: locker} +} + +// IsMine returns true if and only if the passed lock is the same as the one which constructed the lock guard. +func (lg *LockGuard[T]) IsMine(locker sync.Locker) bool { + return lg.l == locker +} + +// Fault returns true if and only if the passed lock isn't the original one we used to construct the lock guard. +func (lg *LockGuard[T]) Fault(locker sync.Locker) bool { + return lg.l != locker +}