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

Jump to bottom

runtime: lock ordering problem between pollDesc and prof #57637

Closed
bcmills opened this issue Jan 5, 2023 · 8 comments
Closed

runtime: lock ordering problem between pollDesc and prof #57637

bcmills opened this issue Jan 5, 2023 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.19.6

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 5, 2023 •
edited 

#!watchflakes
post <- `lock ordering problem` && `^\d+ : pollDesc ` && `\d+ : prof `

From #55167 (comment):

60879  ======
0 : pollDesc 8 0x7fdcf0642a90
1 : prof 22 0x84c5c0
fatal error: lock ordering problem
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 5, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Jan 5, 2023

(attn @golang/runtime)

@bcmills bcmills added this to the Go1.21 milestone Jan 5, 2023
@gopherbot
Copy link

Found new dashboard test flakes for:

#!watchflakes
post <- `lock ordering problem` && `^\d+ : pollDesc ` && `\d+ : prof `
2022-12-19 21:46 linux-amd64-staticlockranking go@6aa1e6d5 net.TestConcurrentSetDeadline (log) 
60879  ======
0 : pollDesc 8 0x7fdcf0642a90
1 : prof 22 0x84c5c0
fatal error: lock ordering problem

runtime stack:
runtime.throw({0x67a36f?, 0x0?})
	/workdir/go/src/runtime/panic.go:992 +0x71 fp=0x7fdce8ff8de0 sp=0x7fdce8ff8db0 pc=0x438c11
runtime.checkRanks(0xc0005f3a00, 0x0?, 0x81bd78?)
	/workdir/go/src/runtime/lockrank_on.go:151 +0x205 fp=0x7fdce8ff8e40 sp=0x7fdce8ff8de0 pc=0x40d985
runtime.lockWithRank.func1()
	/workdir/go/src/runtime/lockrank_on.go:78 +0x87 fp=0x7fdce8ff8e70 sp=0x7fdce8ff8e40 pc=0x40d607
runtime.systemstack()
	/workdir/go/src/runtime/asm_amd64.s:469 +0x49 fp=0x7fdce8ff8e78 sp=0x7fdce8ff8e70 pc=0x468629

runtime.gopark(0x48?, 0x668fa0?, 0x40?, 0x34?, 0x40ffa5?)
	/workdir/go/src/runtime/proc.go:361 +0xd6 fp=0xc00048dda8 sp=0xc00048dd88 pc=0x43b736
runtime.goparkunlock(...)
	/workdir/go/src/runtime/proc.go:367
runtime.semacquire1(0xc0000cef48, 0xd8?, 0x1, 0x0)
	/workdir/go/src/runtime/sema.go:144 +0x1fb fp=0xc00048de10 sp=0xc00048dda8 pc=0x44c53b
sync.runtime_Semacquire(0xc00008a6d8?)
	/workdir/go/src/runtime/sema.go:56 +0x25 fp=0xc00048de40 sp=0xc00048de10 pc=0x466bc5
sync.(*WaitGroup).Wait(0xc00008a618?)
	/workdir/go/src/sync/waitgroup.go:136 +0x52 fp=0xc00048de68 sp=0xc00048de40 pc=0x472292
net.TestConcurrentSetDeadline(0xc0005f3380)
	/workdir/go/src/net/timeout_test.go:1152 +0x3df fp=0xc00048df70 sp=0xc00048de68 pc=0x5f295f
testing.tRunner(0xc0005f3380, 0x688ae8)

— watchflakes

@prattmic
Copy link
Member

prattmic commented Jan 9, 2023

Note that this failure is on release-branch.go1.18.

I believe this was fixed by https://go.dev/cl/418715. pollDesc < prof was already an allowed order, but the direct edge was not listed in the partial order. https://go.dev/cl/418715 adds it to the partial order.

I could make a minimal backport if desired, but given this has only triggered once, I'm not sure it is worth it.

@prattmic prattmic modified the milestones: Go1.21, Go1.18.10 Jan 9, 2023
@prattmic prattmic added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed release-blocker NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 9, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Jan 9, 2023

Given the relatively short remaining shelf-life of Go 1.18, I agree that this probably isn't worth backporting. Thanks for looking into it!

@prattmic prattmic modified the milestones: Go1.18.10, Go1.19.5 Jan 9, 2023
@prattmic
Copy link
Member

prattmic commented Jan 9, 2023

This bug affects 1.19 as well, not sure if that changes your thoughts.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 9, 2023

Hmm. We're going to have 1.19 around for a whole release cycle (so it might be worth fixing to avoid spurious failures during patch releases), but this failure mode is also rare (end users probably never see it, because they don't build with GOEXPERIMENT=staticlockranking).

So, I'm personally ambivalent about backporting. 🤷‍♂️

@aclements
Copy link
Member

I don't consider GOEXPERIMENT=staticlockranking to be a user-facing build mode, and given that this was merely a missing edge in the old lock graph and not an actual order violation, I don't think we should bother with a fix to 1.19 unless the failures are bothering us.

@gopherbot gopherbot modified the milestones: Go1.19.5, Go1.19.6 Jan 10, 2023
@cherrymui
Copy link
Member

Sounds like we decide to not backport. Closing for now. If it actually bothers us we can reopen and backport.

@cherrymui cherrymui closed this as not planned Won't fix, can't repro, duplicate, stale Jan 11, 2023
@golang golang locked and limited conversation to collaborators Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Test Flakes
Status: Done
Milestone
Go1.19.6
Development

No branches or pull requests
5 participants
@prattmic @aclements @bcmills @gopherbot @cherrymui