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

Don't access loop variables in goroutines directly #7188

Merged
merged 3 commits into from
Nov 22, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Nov 21, 2022

Inspired by #7186.
I checked all anonymous goroutines to make sure they don't access any loop or other temporary variables.
This PR fixes the found instances and also contains some readability improvements to visually highlight defer statements better.

Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Nice!!

This commit makes sure that no loop variables or other temporary
variables are accessed directly in a goroutine but are instead passed
into the goroutine through a parameter. This makes sure a copy of the
value is put on the stack and is not changed while the outside loop
continues.
This commit fixes the readability of some of the defer calls in
goroutines by making sure the defer stands out properly.
@Crypt-iQ
Copy link
Collaborator

I think the ones that don't iterate in a for loop are fine - see comment #7186 (comment)

Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

I think @Crypt-iQ is correct and sorry that I gave the wrong reason to make this change🙈

Nonetheless, I find using an anonymous goroutine inside a select statement wrapped in a for loop can be very error-prone, as we are secretly accessing variables inside a loop and may cause the bug. So although the reason is wrong in the first place, I still think it's a better pattern to pass variables into the anonymous functions so we don't need to raise such a concern. Or, even better, avoid using anonymous goroutines, which can also benefit our unit tests.

I think the original post said it well,

One of these two changes is unnecessary and the other is a real bug fix, but you can’t tell which is which without more context.

discovery/gossiper.go Show resolved Hide resolved
@guggero
Copy link
Collaborator Author

guggero commented Nov 22, 2022

I think the ones that don't iterate in a for loop are fine.

I agree after taking a look at your code examples in the referenced PR. I also wasn't sure if the type conversion of the switch already put things on the stack. But I still feel like this more explicit version also puts more emphasis on the goroutine using a variable from outside of its context. So I'm going to merge this anyway.

@guggero guggero merged commit ca501d9 into lightningnetwork:master Nov 22, 2022
@guggero guggero deleted the go-func-vars branch December 1, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants