Skip to content

go/types: data race due to lazy interface completion #47726

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 Aug 16, 2021 · 6 comments
Closed

go/types: data race due to lazy interface completion #47726

findleyr opened this issue Aug 16, 2021 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Milestone
Go1.18

Comments

@findleyr
Copy link
Member

Sample failure:

https://build.golang.org/log/66aed38eb7030ee6db09e17179d26ce42d59b316

Lazy interface completion causes data races in x/tools. We need to either make interface completion concurrency-safe, or ensure that all interfaces that escape the Checker have been completed.

CC @griesemer
@findleyr findleyr added NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) labels Aug 16, 2021
@findleyr findleyr added this to the Go1.18 milestone Aug 16, 2021
@griesemer
Copy link
Contributor

Is this because Interface.Complete is not doing anything anymore? That is, would it be sufficient to revert that change? Or is it a bigger problem?

@findleyr
Copy link
Member Author

Is this because Interface.Complete is not doing anything anymore? That is, would it be sufficient to revert that change? Or is it a bigger problem?

Necessary but not sufficient. Here's a failure at the dev.typeparams merge:
https://build.golang.org/log/53e6ff359c770b04ed89035e59f2038b3d87268e

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/342710 mentions this issue: Revert "go/types: make Interface.Complete a no-op"

@findleyr findleyr mentioned this issue Aug 16, 2021
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/342749 mentions this issue: go/types: check if the interface is already complete in Complete

gopherbot pushed a commit that referenced this issue Aug 17, 2021
@findleyr
Revert "go/types: make Interface.Complete a no-op"
91a935e 
This reverts commit fda8ee8.

Reason for revert: Interface.Complete is still necessary for safe concurrency.

For #47726

Change-Id: I8b924ca5f4af8c7d7e2b5a27bb03a5a5ed9b1d22
Reviewed-on: https://go-review.googlesource.com/c/go/+/342710
Trust: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 17, 2021
@findleyr
go/types: check if the interface is already complete in Complete
29ec74f 
Once Interfaces have been completed they must never be written again,
as they may be used concurrently.

Avoid writing Interface.complete unnecessarily in Complete. Also, update
documentation to reflect that Complete must be called before the
Interface may be considered safe for concurrent use.

For #47726

Change-Id: Ic9fd1395ab0dd6d3499f7a698dadf315abcddab8
Reviewed-on: https://go-review.googlesource.com/c/go/+/342749
Trust: Robert Findley <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
@findleyr
Copy link
Member Author

I believe this is fixed now, but I'll leave it open for visibility in case there are any additional flakes in the race builders.

Will close tomorrow if no additional failures.

@findleyr
Copy link
Member Author

Closing as the build dashboard looks clean.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Projects
None yet
Milestone
Go1.18
Development

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