Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/telemetry/internal/counter: persistent test failures with "panic: runtime error: invalid memory address or nil pointer dereference" on GOOS=android GOARCH={386,amd64} via Android emulator builders #60967

Open
dmitshur opened this issue Jun 23, 2023 · 9 comments
Assignees
Labels
mobile Android, iOS, and x/mobile NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Android telemetry x/telemetry issues
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jun 23, 2023

#!watchflakes
post <- goos == "android" && pkg == "golang.org/x/telemetry/internal/counter" && `panic: runtime error: invalid memory address`

The android-386-emu and android-amd64-emu builders are persistently failing on TestBasic in this package. Example failure:

--- FAIL: TestBasic (0.00s)
    counter_test.go:31: GOOS android GARCH amd64
    counter_test.go:40: counter: disabled by GOTELEMETRY=off
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x7a7d97f5c2fb]

goroutine 19 [running]:
testing.tRunner.func1.2({0x7a7d97fac680, 0x7a7d9809bb70})
	/workdir/go/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/workdir/go/src/testing/testing.go:1548 +0x397
panic({0x7a7d97fac680?, 0x7a7d9809bb70?})
	/workdir/go/src/runtime/panic.go:914 +0x21f
golang.org/x/telemetry/internal/counter.close(0x7a7d97f1230a?)
	/workdir/gopath/src/golang.org/x/telemetry/internal/counter/counter_test.go:73 +0x1b
runtime.Goexit()
	/workdir/go/src/runtime/panic.go:523 +0x145
testing.(*common).FailNow(0xc000086b60)
	/workdir/go/src/testing/testing.go:999 +0x4a
testing.(*common).Fatal(0xc000086b60, {0xc00004af20?, 0xf?, 0xc000036730?})
	/workdir/go/src/testing/testing.go:1076 +0x54
golang.org/x/telemetry/internal/counter.TestBasic(0xc000086b60)
	/workdir/gopath/src/golang.org/x/telemetry/internal/counter/counter_test.go:40 +0x1bc
testing.tRunner(0xc000086b60, 0x7a7d97fcd6e0)
	/workdir/go/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
	/workdir/go/src/testing/testing.go:1648 +0x3ad
FAIL	golang.org/x/telemetry/internal/counter	74.161s

See https://build.golang.org/log/cc69cc12aae4e352100a3ec136d1eb8443ea7b9f for an example log. More are visible at https://build.golang.org/?repo=golang.org%2fx%2ftelemetry.

CC @golang/android as port owners.

If package owners (CC @jamalc, @pjweinb, @hyangah) decide this repository isn't meant to support GOOS=android and doesn't need to be tested on this builder, the right fix is to adjust the test policy in x/build/dashboard. I'm happy to help with that if that's the path to resolve this issue. Thanks.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. mobile Android, iOS, and x/mobile telemetry x/telemetry issues labels Jun 23, 2023
@dmitshur dmitshur added this to the Unreleased milestone Jun 23, 2023
@gopherbot
Copy link

Change https://go.dev/cl/505580 mentions this issue: internal/counter: skip tests on solaris, android, and GOARCH=386

gopherbot pushed a commit to golang/telemetry that referenced this issue Jun 23, 2023
Updates golang/go#60615
Updates golang/go#60692
Updates golang/go#60965
Updates golang/go#60967
Updates golang/go#60968
Updates golang/go#60970
Updates golang/go#60971

Change-Id: Ifb0320c279e91185ab04c3efa6bf20f2c141dbe1
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/505580
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
@gopherbot
Copy link

Change https://go.dev/cl/505720 mentions this issue: internal/counter: avoid nil check panic

@hyangah
Copy link
Contributor

hyangah commented Jun 23, 2023

:-) this revealed a bug in the test code.

But the root cause of this failure is that the telemetry package couldn't create a telemetry collection directory in the android builder environment, so telemetry was disabled. https://github.com/golang/telemetry/blob/b6b6ba5c0fe03deba73c9244d30c283757992cfa/types.go#L82

The failed test assumed telemetry was enabled and eventually hit the bug in the test.

From an old log I see "mkdir /.config: read-only file system" error. The telemetry code attempts to create telemetry data under os.UserConfigDir and on android, it must be a read-only directory.

  • Is os.UserConfigDir a right place to place collected telemetry data?

Since some code will end up in the go main project, I think it's better to keep running on android and skip individual tests that don't make sense, as we discover.

gopherbot pushed a commit to golang/telemetry that referenced this issue Jun 23, 2023
Some tests in this package attempt to close/unmmap files
by calling a test helper function 'close'.
When telemetry is off, mapped file, not the file, can be nil.
Actually, the file argument (f) of close is never nil.

Updates golang/go#60967

Change-Id: I4ca3feb233ede3a6512a4f3d50460c2fb28a6559
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/505720
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Jun 26, 2023

The telemetry code attempts to create telemetry data under os.UserConfigDir and on android, it must be a read-only directory.

Oof. Compare:

But also: it seems like a mistake for the telemetry tests to write to the user's shared telemetry cache — I would assume that we don't want the tests to contaminate the real telemetry-collection backend.

That sort of use case is exactly what the new testing.Testing() function is for — perhaps the telemetry package should use that to disable use of the default location?

@hyangah
Copy link
Contributor

hyangah commented Jun 26, 2023

But also: it seems like a mistake for the telemetry tests to write to the user's shared telemetry cache — I would assume that we don't want the tests to contaminate the real telemetry-collection backend.

cc @jamalc @pjweinb

@gopherbot
Copy link

Change https://go.dev/cl/506917 mentions this issue: all: delay creation of the telemetry dirs until counter.Open

@gopherbot
Copy link

Change https://go.dev/cl/506916 mentions this issue: internal/telemetry: centralize telemetry settings (directories and mode)

gopherbot pushed a commit to golang/telemetry that referenced this issue Jul 17, 2023
The LocalDir/UploadDir/Enabled in types.go are computed based on
internal/telemetry.{LocalDir,UploadDir,ModeFile}.

Previously, the init function in types.go created the telemetry
directories, which means, simply depending on the package creates
directories in user's system default configuration location. For
example, by running tests in this repo, users may end up having
telemetry directories. Avoid this eager directory creation.

The previous behavior allowed a process to completely skip telemetry
collection if there is no writable disk space, even when the user
didn't turn off the telemetry. However, we guess os.UserConfigDir
misconfiguration is rare, so we drop this behavior.

We introduce the undocumented EXP_GOTELEMETRYDIR env var here.
A process' default telemetry setting uses directories and files under
a directory determined by the EXP_GOTELEMETRYDIR env var, or
os.UserConfigDir.

EXP_GOTELEMETRYDIR env var allows us to test instrumented
binary's behavior without polluting the user's default
telemetry mode file or data collection directory.
This environment variable is intentionally undocumented
because this may change or get dropped.

And, rename LookupMode to Mode.

Updates golang/go#60967

Change-Id: Ie8ad9bf52a9b5d7fe70b92f6c8ccdeda6dbb1a90
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/506916
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr
Copy link
Contributor

findleyr commented Nov 6, 2023

With https://go.dev/cl/538297, I think we will not need to write to UserConfigDir, so assigning to Hana.

@bcmills
Copy link
Contributor

bcmills commented Feb 6, 2024

I think we will not need to write to UserConfigDir

Note that while that applies to android, it is not specific to android.

Compare:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Android, iOS, and x/mobile NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Android telemetry x/telemetry issues
Projects
Status: No status
Development

No branches or pull requests

5 participants