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 <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

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

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")
+	}
+}

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
 }

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
 }