-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: GOEXPERIMENT=loopvar bug #61906
Comments
Semantically treating
Note that that example is completely independent of the loop semantics. (Actually, even heap-allocating isn't enough to guarantee a different address. A full GC could technically happen in between loop iterations, freeing the old heap object and reusing the very same address in the heap. Very unlikely, but theoretically possible.) As a result, I don't think this is a bug. Closing for now. |
okay. How about this one: package main
func main() {
for i, ptr := 0, new(int); i < 3; i, ptr = i + 1, &i {
defer func() {
println(ptr == &i)
}()
}
} What should it print? |
And this: package main
import "sync"
func main() {
const N = 3
var wg sync.WaitGroup
wg.Add(N)
var c = make(chan int, 1)
for i, ptr := 0, new(int); i < N; i, ptr = i + 1, &i {
go func() {
c <- i
println(ptr == &i, ptr, i)
<-c
wg.Done()
}()
}
wg.Wait()
} By my understanding, the loop var |
Both of those are working correctly AFAICT. In the first one, both This applies to the second program as well (and explains why it prints |
Do you mean the If this is true, how to do the synchronizations for the local [edit]: okay. The 3rd clause happens before the execution of the closure new goroutine. [edit 2]: there are no synchronization problems at the start of a step, but there are still synchronization problems at the time of end of a step. (And this problem is unrelated to the above discussions. I means how the |
Updated the last comment. |
EDIT: This is incorrect. |
Sorry, no. I was interpreting the semantics incorrectly. The spec at https://go.googlesource.com/proposal/+/master/design/60078-loopvar.md#language-specification is correct. The loop must do the update after the loop body to capture any changes to the loop variable, but that update always happens on the new variable. As a result, I believe your example would be racy if the closure updated
One could argue that it would be nice if captures always made a full copy, but in the language spec they do not. And the new loop semantics are consistent with that. |
But, with the old semantics, it is clear that the cause of the racy situation is users' mistakes. No people will argue on this. With the new semantics, it is not obvious that it is the users' faults, because the assignment In my honest opinion, the benefits of the change is tiny and rare, whereas the drawbacks are too many. BTW, you ever said one reason for this change to make a consistency between |
To be clear, I'm not trying to argue the merits of the new semantics, just whether there's a bug here or not. I got confused because your post earlier implied the design suggested synchronized semantics, but it in fact does not (I have not been following the discussion too closely). AFAICT, the implementation follows the spec (and there doesn't appear to be some glaring hole in the spec, at least to me), so there's nothing left to do here.
FWIW I don't recall participating in the loop variable discussion. If you'd like to continue the discussion, a better place to do that would be either the existing proposal issue #60078 or the golang-dev and/or golang-nuts mailing lists. |
Ah, it might be another people in the compile/runtime team and I mistook it is you. Sorry for the wrong mention.
I didn't say the design makes the guarantee. I just said that it is easy for people to make synchronization mistakes when they modified loop vars in step goroutines but they tend to think there are no mistakes, and this is a drawback of the new semantics. |
And I will not participate any discussions in #60078, for #60078 (comment). I surely will continue the discussions with my twitter account and my blog. |
Indeed, you did not. It was my misunderstanding (and that's all I meant). |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What did you do?
What did you expect to see?
3 false.
What did you see instead?
1 false, 2 true.
The text was updated successfully, but these errors were encountered: