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 <rfindley@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
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 @@
// 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 @@
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 @@
}
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 @@
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 @@
mf.close()
}
})
+ if rotating != rotate {
+ panic("BUG: Open called with inconsistent values for 'rotate'")
+ }
return close
}
