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 #179 #202

Merged
merged 1 commit into from
Nov 19, 2014
Merged

Fix bug #179 #202

merged 1 commit into from
Nov 19, 2014

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Nov 19, 2014

Almost certainly the same as reported at #199 (comment)

Go's closure semantics are really annoying - simply stop spawning goroutines
with the wrong arguments.

Add a test (heavily based on https://gist.github.com/ORBAT/d0adcd790dff34b37b04)
to ensure this behaviour doesn't regress.

Huge thanks to Tom Eklöf for getting me all the logs etc. needed to track this
down.

@wvanbergen @ORBAT

Also reported at #199 (comment)

Go's closure semantics are really annoying - simply stop spawning goroutines
with the wrong arguments.

Add a test (heavily based on https://gist.github.com/ORBAT/d0adcd790dff34b37b04)
to ensure this behaviour doesn't regress.

Huge thanks to Tom Eklöf for getting me all the logs etc. needed to track this
down.
@wvanbergen
Copy link
Contributor

ouch. :shipit:

eapache added a commit that referenced this pull request Nov 19, 2014
@eapache eapache merged commit bdf31b7 into master Nov 19, 2014
@eapache eapache deleted the fix-179 branch November 19, 2014 15:19
@cdmicacc
Copy link

This isn't anything to do with Go's closure semantics. It's simply a misunderstanding about what's going on in a for x := ....

The buggy code can be distilled down to this:

    list := []int{1, 2, 3, 4}
    for x := range list {
        go func() {
            fmt.Printf("x = %d\n", x)
        }()
    }

But you can rewrite that as

    list := []int{1, 2, 3, 4}
    var x int
    for x = range list {
        go func() {
            fmt.Printf("x = %d\n", x)
        }()
    }

In this latter form it's a lot easier to see that what you're closing over with x is actually a single spot in memory whose value is being reassigned on each loop iteration. There is no magic in for x:= range that allocates a new x on each loop iteration. In that case it makes perfect sense that each goroutine sees the same value.

The fix would really be to copy the value, like so:

    list := []int{1, 2, 3, 4}
    for x := range list {
        go func(x int) {
            fmt.Printf("x = %d\n", x)
        }(x)
    }

Since Go passes by value, that will result in a copy of the current value of x being preserved in the goroutine, rather than x itself.

@eapache
Copy link
Contributor Author

eapache commented Nov 19, 2014

I consider it part of Go's closure semantics that it closes by reference and not by value.

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.

3 participants