Fix slice gotchas including concurrency race

If the []Attr slice passed to Conn.Entry was used in several concurrent
calls, and it had sufficient capacity that append()ing to it succeeded
without reallocating the slice, then a data race would exist.

Fix this, and fix the efficiency gotcha which expected the attribute
slice to have additional capacity, by instead always constructing a new
final slice of []Attr and using a sync.Pool to avoid reallocating them.

Fixes go test -race, and improves the benchmark. Before:
	goos: linux
	goarch: amd64
	pkg: src.lwithers.me.uk/go/journal
	cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
	BenchmarkEntry-8   	8345163	    2139 ns/op	   848 B/op	    15 allocs/op
	PASS
	ok  	src.lwithers.me.uk/go/journal	20.034s

After:
	goos: linux
	goarch: amd64
	pkg: src.lwithers.me.uk/go/journal
	cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
	BenchmarkEntry-8   	12611158	    1445 ns/op	   221 B/op	     7 allocs/op
	PASS
	ok  	src.lwithers.me.uk/go/journal	19.673s
This commit is contained in:
Laurence Withers 2024-02-18 14:14:31 +00:00
parent 0c0a814a4e
commit d3666c9152
1 changed files with 20 additions and 8 deletions

28
conn.go
View File

@ -2,6 +2,7 @@ package journal
import ( import (
"net" "net"
"slices"
"sync" "sync"
) )
@ -23,6 +24,7 @@ type Conn struct {
s *net.UnixConn s *net.UnixConn
bufs sync.Pool bufs sync.Pool
atts sync.Pool
} }
// Connect to the systemd journal. If the path string is empty, then it uses the // Connect to the systemd journal. If the path string is empty, then it uses the
@ -40,23 +42,26 @@ func Connect(path string) (*Conn, error) {
return nil, err return nil, err
} }
return &Conn{ c := &Conn{
s: s, s: s,
bufs: sync.Pool{ bufs: sync.Pool{
New: func() any { New: func() any {
return &net.Buffers{} return &net.Buffers{}
}, },
}, },
}, nil }
c.atts = sync.Pool{
New: func() any {
return make([]Attr, 0, 2+len(c.Common))
},
}
return c, nil
} }
var messageAttrKey = AttrKey{key: "MESSAGE"} var messageAttrKey = AttrKey{key: "MESSAGE"}
// Entry emits a log entry. It will add PRIORITY and MESSAGE key/value pairs // Entry emits a log entry. It will add PRIORITY and MESSAGE key/value pairs
// as well as any Common attributes. // as well as any Common attributes.
//
// Note: to avoid allocation / garbage, ensure attrs has capacity for an extra
// 2+len(c.Common) values.
func (c *Conn) Entry(pri Priority, msg string, attrs []Attr) { func (c *Conn) Entry(pri Priority, msg string, attrs []Attr) {
err := c.EntryErr(pri, msg, attrs) err := c.EntryErr(pri, msg, attrs)
switch { switch {
@ -72,12 +77,18 @@ func (c *Conn) Entry(pri Priority, msg string, attrs []Attr) {
// EntryErr is like Entry, but will propagate errors to the caller, rather than // EntryErr is like Entry, but will propagate errors to the caller, rather than
// using the built-in error handler. // using the built-in error handler.
func (c *Conn) EntryErr(pri Priority, msg string, attrs []Attr) error { func (c *Conn) EntryErr(pri Priority, msg string, attrs []Attr) error {
attrs = append(attrs, pri.Attr(), Attr{ a := c.atts.Get().([]Attr)
a = slices.Grow(a[:0], 2+len(c.Common)+len(attrs))
a = append(a, pri.Attr(), Attr{
Key: messageAttrKey, Key: messageAttrKey,
Value: []byte(msg), Value: []byte(msg),
}) })
attrs = append(attrs, c.Common...) a = append(a, c.Common...)
return c.WriteAttrs(attrs) a = append(a, attrs...)
err := c.WriteAttrs(attrs)
slices.Delete(a, 0, len(a)) // ensure GC doesn't see refs to old data
c.atts.Put(a)
return err
} }
// WriteAttrs is a low-level method which writes a journal entry comprised of // WriteAttrs is a low-level method which writes a journal entry comprised of
@ -86,6 +97,7 @@ func (c *Conn) WriteAttrs(attrs []Attr) error {
buf := c.bufs.Get().(*net.Buffers) buf := c.bufs.Get().(*net.Buffers)
*buf = (*buf)[:0] *buf = (*buf)[:0]
err := WireWrite(buf, c.s, attrs) err := WireWrite(buf, c.s, attrs)
slices.Delete(*buf, 0, len(*buf)) // ensure GC doesn't see refs to old data
c.bufs.Put(buf) c.bufs.Put(buf)
return err return err
} }