Skip to content

Commit 8e316b9

Browse files
committed
internal/counter: fix flaky test
Quim Montel points out that several raw calls to munmap had incomplete effect and should be replaced by mappedFile.close(). The tests ran 15000 times without any failures. Fixes golang/go#65182 (TestRotate flaky on Windows) Fixes: golang/go#65255 (double panic caused by munmapping twice) Change-Id: Ice445c4d5b521b4a49971eced07bcff1aadb157f Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/559498 Reviewed-by: Robert Findley <[email protected]> Run-TryBot: Peter Weinberger <[email protected]> Reviewed-by: Hyang-Ah Hana Kim <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
1 parent b4bb114 commit 8e316b9

File tree

2 files changed

+6
-12
lines changed

2 files changed

+6
-12
lines changed

internal/counter/counter_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"testing"
2727
"time"
2828

29-
"golang.org/x/telemetry/internal/mmap"
3029
"golang.org/x/telemetry/internal/telemetry"
3130
"golang.org/x/telemetry/internal/testenv"
3231
)
@@ -70,6 +69,7 @@ func TestBasic(t *testing.T) {
7069
}
7170

7271
func TestMissingLocalDir(t *testing.T) {
72+
testenv.SkipIfUnsupportedPlatform(t)
7373
err := os.RemoveAll(telemetry.LocalDir)
7474
if err != nil {
7575
t.Fatal(err)
@@ -130,8 +130,7 @@ func close(f *file) {
130130
// telemetry might have been off
131131
return
132132
}
133-
mmap.Munmap(mf.mapping)
134-
mf.f.Close()
133+
mf.close()
135134
}
136135

137136
func TestLarge(t *testing.T) {

internal/counter/file.go

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"bytes"
99
"errors"
1010
"fmt"
11-
"log"
1211
"math/rand"
1312
"os"
1413
"path"
@@ -277,12 +276,8 @@ func (f *file) rotate1() (expire time.Time, cleanup func()) {
277276
return
278277
}
279278
// now it is safe to clean up the old mapping
280-
if err := previous.f.Close(); err != nil {
281-
log.Print(err)
282-
}
283-
if err := munmap(previous.mapping); err != nil {
284-
log.Print(err)
285-
}
279+
// Quim Montel pointed out the previous coeanup was incomplete
280+
previous.close()
286281
}
287282

288283
name, expire, err := f.filename(counterTime())
@@ -373,8 +368,7 @@ func Open() func() {
373368
// telemetry might have been off
374369
return
375370
}
376-
mmap.Munmap(mf.mapping)
377-
mf.f.Close() // best effort
371+
mf.close()
378372
}
379373
}
380374

@@ -406,6 +400,7 @@ func openMapped(name string, meta string, existing *mappedFile) (_ *mappedFile,
406400
f: f,
407401
meta: meta,
408402
}
403+
// without this files cannot be cleanedup on Windows (affects tests)
409404
runtime.SetFinalizer(m, (*mappedFile).close)
410405
defer func() {
411406
if err != nil {

0 commit comments

Comments
 (0)