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

Address engine data races detected by go test -race #10081

Merged
merged 6 commits into from Jul 12, 2022
Merged

Conversation

AaronFriel
Copy link
Member

These races were detected via Go's race detection tool and are separated by commit.

cd pkg && GOWORK=off go test -race -p 1 --parallel 4 -v ./engine/...

@AaronFriel AaronFriel changed the title Address data races detected by go test -race Address engine data races detected by go test -race Jul 9, 2022
@AaronFriel AaronFriel added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 9, 2022
Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM! Can we adjust the Makefile to run the race detector as part of CI @AaronFriel? That way we don't let new races pop up in this section of the codebase.

sdk/go/pulumi/types.go Outdated Show resolved Hide resolved
@AaronFriel
Copy link
Member Author

AaronFriel commented Jul 11, 2022

@RobbieMcKinstry The race detector dramatically slows down tests, so I think we should do that work as part of a CI overhaul when we have merge queues that allow us to move that later in the process so it's non-blocking for velocity. (Or we find a tailored subset of tests to run with it.)

More anecdata here: https://www.google.com/search?q=go+test+%22race%22+performance+slower+site:github.com&client=firefox-b-1-m

We have a quality bar epic, I'll add merge queues and race detection issues to it.

@RobbieMcKinstry
Copy link
Contributor

I'm forgetting how this is implemented. IIRC it's doing runtime detection, which is why tests slow down. I never understood why the Go team didn't implement an interprocedural analysis for this feature instead.

Thanks for volunteering to add the issue to the quality bar epic. IMO its perfectly fine to add it later when we have more opportunity to optimize its execution around CI bandwidth constraints.

@AaronFriel
Copy link
Member Author

@RobbieMcKinstry Created:

@AaronFriel AaronFriel merged commit 4e19e95 into master Jul 12, 2022
@pulumi-bot pulumi-bot deleted the friel/data-races branch July 12, 2022 16:39
@AaronFriel AaronFriel added kind/bug Some behavior is incorrect or out of spec area/cli UX of using the CLI (args, output, logs) impact/panic This bug represents a panic or unexpected crash labels Jul 12, 2022
@AaronFriel AaronFriel self-assigned this Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli UX of using the CLI (args, output, logs) impact/no-changelog-required This issue doesn't require a CHANGELOG update impact/panic This bug represents a panic or unexpected crash kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants