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

fix(bug): use copy of loop variable in Go routines #8163

Merged
merged 1 commit into from
Sep 20, 2022
Merged

fix(bug): use copy of loop variable in Go routines #8163

merged 1 commit into from
Sep 20, 2022

Conversation

renatolabs
Copy link
Contributor

This fixes cases where loop variables are incorrectly captured in
asynchronous code (parallel tests or Go routines). In the case of a
parallel test, the most likely scenario is that only the last test
case will be executed. In the case of Go routines that directly
access loop variables without any synchronization, the value stored in
the loop variable will likely not be the same as what it was when the
Go routine is created.

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@matthewmcneely matthewmcneely left a comment

Choose a reason for hiding this comment

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

@skrdgraph I recommend that this get merged, good catch by @renatolabs here

@skrdgraph
Copy link
Contributor

@renatolabs thanks for the PR. 👍 to merge this (it does make sense to get this in asap)

I guess we also have similar issues in our t.go.

@skrdgraph
Copy link
Contributor

skrdgraph commented Sep 19, 2022

@renatolabs - could you rebase with latest changes and push again please? We would like to get this in once the tests pass. The rebase should pull in our latest test CI setup.

This fixes cases where loop variables are incorrectly captured in
asynchronous code (parallel tests or Go routines). In the case of a
parallel test, the most likely scenario is that only the last test
case will be executed. In the case of Go routines that directly
access loop variables without any synchronization, the value stored in
the loop variable will likely not be the same as what it was when the
Go routine is created.
@renatolabs
Copy link
Contributor Author

@renatolabs - could you rebase with latest changes and push again please? We would like to get this in once the tests pass. The rebase should pull in our latest test CI setup.

Done!

@skrdgraph
Copy link
Contributor

@renatolabs - could you rebase with latest changes and push again please? We would like to get this in once the tests pass. The rebase should pull in our latest test CI setup.

Done!

Thanks, I have approved a CI run to happen on this PR now. Ill merge it one once it completes.

@skrdgraph skrdgraph changed the title fix: use copy of loop variable in Go routines fix(bug): use copy of loop variable in Go routines Sep 20, 2022
@skrdgraph
Copy link
Contributor

@kevinmingtarja we need to look at why the COVERALLS token was not passed. The tests did pass - I am going to merge this, and if this continues to be an issue, we should look into this.

@skrdgraph skrdgraph merged commit b69b8e5 into dgraph-io:main Sep 20, 2022
@joshua-goldstein
Copy link
Contributor

For future reference I want to add a link to a reference here on Go.dev that mentions this issue. It seems like a common point of confusion in Go, and this PR is indeed idiomatic Go. One could equivalently pass the variables as arguments to the closure but this also works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants