Skip to content

cmd/go, x/telemetry: counter.Open breaks runtime deadlock detection #68497

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.

Sign up for GitHub

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

Jump to bottom
Closed
findleyr opened this issue Jul 17, 2024 · 4 comments
Closed

cmd/go, x/telemetry: counter.Open breaks runtime deadlock detection #68497

findleyr opened this issue Jul 17, 2024 · 4 comments
Assignees
@findleyr
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@findleyr
Copy link
Member

Reported by @cuonglm in #68311: it looks like counter.Open breaks runtime deadlock detection (the all goroutines are asleep...), probably due to the timer to rotate the counter file. This is just a guess -- I'm not sure how that deadlock detection works.
https://go.googlesource.com/telemetry/+/refs/heads/master/internal/counter/file.go#192

#68311 (comment)

CC @golang/telemetry

We should fix this for 1.23. The safest fix is probably to have a separate telemetry.Open call that does not rotate, since all cmd/go invocations should have a short lifespan and no rotation is necessary. @matloob what do you think?

@golang/runtime: is there a trick to set a timer that doesn't affect deadlock detection?
@findleyr findleyr added this to the Go1.23 milestone Jul 17, 2024
@findleyr findleyr mentioned this issue Jul 17, 2024
Closed
@dmitshur dmitshur moved this to Todo in Release Blockers Jul 17, 2024
@findleyr findleyr self-assigned this Jul 17, 2024
@findleyr
Copy link
Member Author

Reproduced and confirmed that avoiding the timer fixes the symptoms.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 17, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/599075 mentions this issue: counter: avoid the rotation timer in counter.Open

@findleyr findleyr added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jul 17, 2024
@findleyr
Copy link
Member Author

Will close this after revendoring telemetry.

Upon further consideration, I don't think this meets the threshold of a release blocker, since it only affects already-broken programs. I still think we should land the above fix for 1.23, however.

gopherbot pushed a commit to golang/telemetry that referenced this issue Jul 17, 2024
@findleyr @gopherbot
counter: avoid the rotation timer in counter.Open
0b706e1 
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]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/598957 mentions this issue: cmd: vendor golang.org/x/telemetry@0b706e1

@github-project-automation github-project-automation bot moved this from Todo to Done in Release Blockers Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@findleyr findleyr

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Blockers
Status: Done
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@gopherbot @cherrymui @findleyr