-
Notifications
You must be signed in to change notification settings - Fork 18k
x/tools/go/packages: race detected during execution of various tests #36605
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
Comments
I believe that the two lower data races were briefly introduced in |
Thanks for the update @stamblerre. I'll retitle this to be about the remaining races in |
Noticed one more that I don't think we've debugged yet:
/cc @heschik |
Change https://golang.org/cl/215319 mentions this issue: |
We used to read the go.mod file information out of the imports.Resolver. Now that gopls tracks go.mod itself, we can use that instead. This is a slight regression, in that go.mods in replace targets will no longer be watched, but I don't think that's too important. This allows us to stop reading the ModuleResolver's internals, which were not sufficiently locked. Updates golang/go#36605. Change-Id: I42939e0248cba1f6b3850a003de67fcc11ab10b1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/215319 Run-TryBot: Heschi Kreinick <heschi@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The above CL fixed the |
I'll take a look at the go/packages races |
The failure in go/packages looks like #34919. (Which explains why it's only occurring in 1.13 -- #34919 has been fixed on tip). The fix is a pretty simple cl: golang.org/cl/201205, but I don't know what the bar is for cherry picking something like that in. go/packages relies on it being safe to do concurrent reads of package types, and changing that would be a pretty invasive change, so I don't think we'd want to go down the route of modifying go/packages. I tried cherrypicking the fix CL (golang.org/cl/201205) and running the tests in race mode and it seems to fix the issue. @dmitshur, What do you think? |
Thanks for the analysis @matloob. I'm not sure if those |
I suspected they wouldn't... the problem is that the user of the code (package |
I understand there is currently a data race in Do you know how big of a change it'd be to update |
This is a race in the go/types API. The only workarounds are to not use the API concurrently, which we can do in go/packages, but which would show up to other users. Fixing this in go/types would require writing new code, so the backport seems preferable. |
Change https://golang.org/cl/227859 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
This is a followup to CL 214433. Start using n1-highcpu-16 machine type instead of n1-highcpu-8 for the freebsd-amd64-race builder. Increasing the RAM from 3.6 GB to 7.2 GB has helped golang/go#36444 significantly: the builder stopped failing consistently on x/tools and resulted in many data races being uncovered in golang/go#36605. However, by now, it has started to fail consistently again. This time it seems to be due to low performance, causing the tests in golang.org/x/tools/internal/lsp/regtest package to fail due with "context deadline exceeded" errors. FreeBSD is one of the ports that stays visible when "show only first- class ports" is checked on build.golang.org. The other -race builders have all been upgraded to the n1-highcpu-16 machine type by now out of necessity. It seems fair to provide the FreeBSD port with an equal amount of resources, even if the increased memory isn't strictly required yet. Once this change is applied, if the failures persist, we can be more confident that the problem is due to the code or the port, rather than due to this -race builder having 2𝗑 less CPU and RAM resources compared to other -race builders. An alternative is to increase timeout for this builder type, but I'm opting to defer exploring that after equalizing the machine type. For golang/go#36444. For golang/go#34621. For golang/go#29252. For golang/go#33986. Change-Id: I41f149365128c7bc6f576c778ac07618acc04612 Reviewed-on: https://go-review.googlesource.com/c/build/+/227859 Reviewed-by: Alexander Rakoczy <alex@golang.org>
It's been a bit longer than two weeks, so closing. 😅 |
From https://build.golang.org/?repo=golang.org%2fx%2ftools, there are a number of instances where the freebsd-amd64-race builder reported a failure. Many of them failed due to data races:
See:
/cc @stamblerre @matloob @findleyr
The text was updated successfully, but these errors were encountered: