From 0b706e19b70189f217f242e6875dc0bb8e64b601 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 17 Jul 2024 17:40:07 +0000 Subject: [PATCH] counter: avoid the rotation timer in counter.Open As observed in golang/go#68497, scheduling a rotation timer can break runtime deadlock detection (all goroutines are asleep...). Furthermore, for short-lived processes such as command line tools, there is no reason to schedule a file rotation. Therefore, change the default behavior of counter.Open not to rotate the counter file, and introduce a new OpenAndRotate API to be used by gopls. For golang/go#68497 Change-Id: I7929c2d622d15e36ca99036d8c7eac1eed8fabf5 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/599075 Auto-Submit: Robert Findley Reviewed-by: Michael Matloob Reviewed-by: Hyang-Ah Hana Kim LUCI-TryBot-Result: Go LUCI --- counter/counter.go | 17 ++++++++++-- counter/counter_test.go | 44 ++++++++++++++++++++++++++++++ counter/countertest/countertest.go | 3 ++ internal/counter/file.go | 26 +++++++++++++++--- 4 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 counter/counter_test.go diff --git a/counter/counter.go b/counter/counter.go index ff727ad..fe2d0f6 100644 --- a/counter/counter.go +++ b/counter/counter.go @@ -83,8 +83,21 @@ func NewStack(name string, depth int) *StackCounter { // If the telemetry mode is "off", Open is a no-op. Otherwise, it opens the // counter file on disk and starts to mmap telemetry counters to the file. // Open also persists any counters already created in the current process. +// +// Open should only be called from short-lived processes such as command line +// tools. If your process is long-running, use [OpenAndRotate]. func Open() { - counter.Open() + counter.Open(false) +} + +// OpenAndRotate is like [Open], but also schedules a rotation of the counter +// file when it expires. +// +// See golang/go#68497 for background on why [OpenAndRotate] is a separate API. +// +// TODO(rfindley): refactor Open and OpenAndRotate for Go 1.24. +func OpenAndRotate() { + counter.Open(true) } // OpenDir prepares telemetry counters for recording to the file system, using @@ -97,7 +110,7 @@ func OpenDir(telemetryDir string) { if telemetryDir != "" { telemetry.Default = telemetry.NewDir(telemetryDir) } - counter.Open() + counter.Open(false) } // CountFlags creates a counter for every flag that is set diff --git a/counter/counter_test.go b/counter/counter_test.go new file mode 100644 index 0000000..999f4dd --- /dev/null +++ b/counter/counter_test.go @@ -0,0 +1,44 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package counter_test + +import ( + "os" + "os/exec" + "testing" + + "golang.org/x/telemetry/counter" + "golang.org/x/telemetry/internal/telemetry" + "golang.org/x/telemetry/internal/testenv" +) + +const telemetryDirEnvVar = "_COUNTER_TEST_TELEMETRY_DIR" + +func TestMain(m *testing.M) { + if dir := os.Getenv(telemetryDirEnvVar); dir != "" { + // Run for TestOpenAPIMisuse. + telemetry.Default = telemetry.NewDir(dir) + counter.Open() + counter.OpenAndRotate() // should panic + os.Exit(0) + } + os.Exit(m.Run()) +} + +func TestOpenAPIMisuse(t *testing.T) { + testenv.SkipIfUnsupportedPlatform(t) + + // Test that Open and OpenAndRotate cannot be used simultaneously. + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + cmd := exec.Command(exe) + cmd.Env = append(os.Environ(), telemetryDirEnvVar+"="+t.TempDir()) + + if err := cmd.Run(); err == nil { + t.Error("Failed to detect API misuse: no error from calling both Open and OpenAndRotate") + } +} diff --git a/counter/countertest/countertest.go b/counter/countertest/countertest.go index dc8bb11..533f5e5 100644 --- a/counter/countertest/countertest.go +++ b/counter/countertest/countertest.go @@ -40,6 +40,9 @@ func Open(telemetryDir string) { } telemetry.Default = telemetry.NewDir(telemetryDir) + // TODO(rfindley): reinstate test coverage with counter rotation enabled. + // Before the [counter.Open] and [counter.OpenAndRotate] APIs were split, + // this called counter.Open (which rotated!). counter.Open() opened = true } diff --git a/internal/counter/file.go b/internal/counter/file.go index 56fa516..5df6dd7 100644 --- a/internal/counter/file.go +++ b/internal/counter/file.go @@ -357,27 +357,42 @@ func (f *file) newCounter1(name string) (v *atomic.Uint64, cleanup func()) { return v, cleanup } -var openOnce sync.Once +var ( + openOnce sync.Once + // rotating reports whether the call to Open had rotate = true. + // + // In golang/go#68497, we observed that file rotation can break runtime + // deadlock detection. To minimize the fix for 1.23, we are splitting the + // Open API into one version that rotates the counter file, and another that + // does not. The rotating variable guards against use of both APIs from the + // same process. + rotating bool +) // Open associates counting with the defaultFile. // The returned function is for testing only, and should // be called after all Inc()s are finished, but before // any reports are generated. // (Otherwise expired count files will not be deleted on Windows.) -func Open() func() { +func Open(rotate bool) func() { if telemetry.DisabledOnPlatform { return func() {} } close := func() {} openOnce.Do(func() { + rotating = rotate if mode, _ := telemetry.Default.Mode(); mode == "off" { // Don't open the file when telemetry is off. defaultFile.err = ErrDisabled // No need to clean up. return } - debugPrintf("Open") - defaultFile.rotate() + debugPrintf("Open(%v)", rotate) + if rotate { + defaultFile.rotate() // calls rotate1 and schedules a rotation + } else { + defaultFile.rotate1() + } close = func() { // Once this has been called, the defaultFile is no longer usable. mf := defaultFile.current.Load() @@ -388,6 +403,9 @@ func Open() func() { mf.close() } }) + if rotating != rotate { + panic("BUG: Open called with inconsistent values for 'rotate'") + } return close }