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

runtime: throw + allglock ordering problem #42669

Open
prattmic opened this issue Nov 17, 2020 · 4 comments
Open

runtime: throw + allglock ordering problem #42669

prattmic opened this issue Nov 17, 2020 · 4 comments
Labels
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Nov 17, 2020

throw -> dopanic_m -> tracebackothers takes allglock. Since pretty much any lock can be held during throw, this is almost always a lock ordering problem. In fact, staticlockranking builders almost always report this ordering issue while crashing due to another ordering issue.

More importantly, if allglock is held when throwing, we will deadlock in the throw. Most of the throws immediately near locking allglock unlock before throw to avoid this issue. However we also call into the allocator with allglock held, and throws in the allocator don't drop allglock and will thus still deadlock.

One option (thanks @mknyszek) is to simply not take allglock during fatal throw/panic since freezetheworld should stop racing threads. freezetheworld doesn't make guarantees, so this would still have the possibility of corruption, but that may be better than deadlock on throw?

cc @danscales @aclements

@prattmic prattmic added the NeedsFix label Nov 17, 2020
@prattmic prattmic added this to the Backlog milestone Nov 17, 2020
@danscales
Copy link

@danscales danscales commented Nov 17, 2020

I don't know the guarantees/non-guarantees of freezetheworld, but it seems like the current chance of deadlock is better than any chance of corruption as we're going through the process of throwing.

Regardless of what decision is made, it may be worth putting a reference to this bug and allglock's role during throw in lockrank.go. I think I meant to make a comment about allglock in the lock rank code, but then forgot to do it.

@prattmic
Copy link
Member Author

@prattmic prattmic commented Nov 17, 2020

I think I meant to make a comment about allglock in the lock rank code, but then forgot to do it.

This is actually mentioned, though a bit indirectly: https://cs.opensource.google/go/go/+/master:src/runtime/lockrank_on.go;l=69;drc=420c68dd68c648af6642dd7e5cf6dacf9f067f6e

I was definitely primarily motivated here by annoyance at the allglock ordering issue that always pops up after reporting another issue. But we can't just suppress it since it is a real issue.

I definitely split on deadlock vs corruption: corruption is definitely worse, but I don't know how frequently freezetheworld fails to stop things in time. I suspect we don't really know how reliable it is because typically other threads failing to stop wouldn't cause any problems with the throw.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 17, 2020

Change https://golang.org/cl/270861 mentions this issue: runtime: don't take allglock during fatal throw/panic

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 17, 2020

Change https://golang.org/cl/270862 mentions this issue: runtime: update paniclk ordering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.