7 ms·

That's one thing I hate about Go, is how easy it is to shoot yourself in the foot with it. Really, it has a decent way to manage inter-thread communication - c

That's one thing I hate about Go, is how easy it is to shoot yourself in the foot with it. Really, it has a decent way to manage inter-thread communication - channels. But the language still permits me to read/write a "non const" global variable from a goroutine. This is horrible, especially when refactoring. Each goroutine should have it's own scope (unless explicitly defined).

atombender 10y ago
More than once I've also had to kick myself for making the rookie mistake of closing over a loop variable. So something like: for _, v := range values { go func() { // Do stuff with v } } Instead of: for _, v := range values { go (func(v string) { // Do stuff with value })(v) } It's rare enough, and subtle enough, that every instance tends to result in 5-10 minutes of puzzled debugging with stdout printing statements until I realize what's going on. What does Rust do here? I like to say that Go's strictness is unevenly distributed. Unused imports are illegal, but Go is perfectly happy to let you shadow variables, have closure use loop variables, or reassign the built-in values ("nil", "true", etc.). It's like a parent who locks the scissors away in a drawer but doesn't mind leaving drain cleaner on the kitchen table.
- Jabbles 10y ago
  You should use go vet https://golang.org/cmd/vet/#hdr-Struct_tags https://golang.org/cmd/vet/#hdr-Struct_tags (There seems to be a documentation error.) And try the race detector: https://golang.org/doc/articles/race_detector.html https://golang.org/doc/articles/race_detector.html Though of course, it's hard to argue that it would be nice to prevent these from compiling.
  - dispose13432 10y ago
    Well, that's an "easy" bug. What about not-shadowing a "non-loop" variable? Take a look at this question: http://stackoverflow.com/questions/18499352/golang-cuncurrency-how-to-append-to-the-same-slice-from-different-goroutines http://stackoverflow.com/questions/18499352/golang-cuncurren... It's easy to mess up during refactoring, and there's pretty much no reason to allow it.
  - lobster_johnson 10y ago
    Indeed, it's invaluable. I use "go vet" and have enabled all of the Gometalinter linters [1] that make sense. "go vet" never warned me about goroutine closure errors, not sure why, maybe I was running an old version. But I'm glad that it's supported. That said, some of the things that "go vet" catches should, in my opinion, be errors. [1] https://github.com/alecthomas/gometalinter https://github.com/alecthomas/gometalinter
- dilap 10y ago
  I feel like this specific issue Go just got wrong. 99% of the time you want a new variable in the range loop. I think Go's strictness is very practical -- it's strict where they saw real problems and the cure was easy. Something like threading race conditions is a real problem, but not easy to fix. Go does give you a pretty good runtime tool with the race checker, though. It would quickly catch something like the fish bug of grabbing the wrong lock.
  - dispose13432 10y ago
    >Something like threading race conditions is a real problem, but not easy to fix. It is. It's called channels.
    - dilap 10y ago
      Sure, you could only use channels and you'd never get races, but in practice that'd be unwieldy and slow, which is why people write traditional lock-based sync stuff in Go all the time, and depend on old-fashioned debugging (and the race-checker). Rust really fixes this (with its ownership system), but it wasn't easy.
      - dispose13432 10y ago
        >unwieldy and slow So force one to do it consciously (kind of like unsafe).
      - masklinn 10y ago
        > Sure, you could only use channels and you'd never get races Go allows sending pointers to non-locked structures over channels, so it's quite easy to "only use channels" and still get race. You can also hit that issue if you spawn multiple goroutines sharing the same initial lexical environment if they make use of a mutable structure from that environment.
        dilap 10y ago
        If you're accessing mutable data from multiple goroutines, then you're not only using channels! :)
- Animats 10y ago
  Shadowing a variable really ought to be an error. At least, you shouldn't be able to hide a local variable with another local variable. Yes, sometimes you have to write "vv" in the inner loop instead of "v", and not feel as l33t. Deal with it. (A shadow variable problem in C just turned up in firmware for a surface mount reflow soldering oven I have. The variable name is "avgtemp". This may explain why some ovens scorch PC boards.[1] Read the discussion. Note that none of the people writing about the issue understand that "static" would confine the scope to one file, and that uninitialized variable declarations at top level are global to the whole program. That's probably because they came up from Arduino land, where people are not taught to think about that stuff. Arduino land is really full C++ using gcc, but it's not taught that way.) [1] https://github.com/UnifiedEngineering/T-962-improvements/issues/90 https://github.com/UnifiedEngineering/T-962-improvements/iss...
  - jjnoakes 10y ago
    I don't think the parent is talking about problems with shadowing.
    - Animats 10y ago
      They wrote "but Go is perfectly happy to let you shadow variables...", although their main issue was with closures.
      - jjnoakes 10y ago
        Whoops, right you are!
  - vram22 10y ago
    >Note that none of the people writing about the issue understand that "static" would confine the scope to one file That seems a bit surprising, unless they did not know C even reasonably well. IIRC (though I haven't used C much lately, I did a lot with it earlier), that is a not-too-advanced feature of the C language. I think it is covered in the K&R C book, near the middle or in the latter half (don't have it handy right now to check).
- electrum 10y ago
  Fortunately, that mistake isn't possible in Java with lambda expressions or anonymous classes. It is a compile error if any captured variables are not "effectively final". Prior to Java 8, which introduced lambdas, variables used with anonymous classes were required to be final. That restriction is now relaxed as the compiler can infer it. Interestingly, the for-each loop variable is effectively final, unless you explicitly modify it, so this code is legal (and correct): for (String v : values) { executor.execute(() -> System.out.println(v)); }
  - sgift 10y ago
    I can never decide if enforcing (effectively) final is a good feature or a bad one. Sure, you cannot shoot yourself in the foot with these limits, on the other hand you loose all the power of real closures.
    - twblalock 10y ago
      You can easily modify variables outside of the stream. For example, to increment a counter declared outside of the lambda, use a final AtomicInteger or similar class instead of an int. For any other value, use a final class that wraps it. The point of enforcing final variables is to prevent programmers from accidentally modifying things they don't want to. It does not prevent programmers from modifying variables intentionally.
      - sgift 10y ago
        I know very well that I can use inner mutability to work around that restriction, that doesn't change the fact that it is a workaround, nothing more.
        twblalock 10y ago
        It's not a workaround. It was designed to work that way on purpose. The people who developed this feature could have easily prevented programmers from using any variables outside of the stream, final or not, but they chose not to because they wanted to allow programmers to use final variables in this way. Furthermore, it does not cause you to lose "all the power of real closures" like you said it does. All you lose is the ability to use a closure around non-final variables, which is a trivial drawback in any use case I've ever come across. You lose only a little bit of the power of closures.
- tatterdemalion 10y ago
  I don't know Go, so I don't really understand what the issue is with the code you posted. In Rust, the closure used to spawn a new thread needs to own its environment; if it does then there's no problem. If it doesn't that's a data race and you have a compile time error about it.
  - lobster_johnson 10y ago
    The error in the Go snippet is that "v" is a single memory location shared throughout the loop. The closure doesn't get a copy, so what usually ends up happening is that every goroutine gets the last value (since the goroutines are probably not scheduled until the end of the loop). This problem doesn't just affect loops, of course: A goroutine can access any local environment outside its scope, and local variables can mutate. The worst surprise I ever encountered was this (simplified): func makeWorkersDocomplicatedStuff() ch := make(chan string) defer func() { if ch != nil { close(ch) } } for i := 0; i < numWorkers; i++ { go func() { for { select { case s := <-ch: // ... } } } } close(ch) ch = nil // ... More stuff ... } What will happen here is that the goroutines will all block forever, because "ch" becomes nil, and in Go, polling a nil channel will block forever (it's an interesting design choice in such a strict language). Rewriting and refactoring this was trivial enough, but catching it was time wasted. Lessons learned: (1) Be scrupulous about closure environments, (2) be super careful about nil channels, and (3) try to avoid defers whose concerns don't fully encapsulate the function body (so defers in the middle of a function is often code smell).
    - tatterdemalion 10y ago
      > The error in the Go snippet is that "v" is a single memory location shared throughout the loop. In Rust this is not true. However, the same concept applies - say it was a variable from outside the loop. In that case, you would get a clear compile time error about moving the value into the thread's closure more than once. A similar error would apply if you iterated over a container by reference (instead of by value).
- kibwen 10y ago
  > What does Rust do here? This code results in a compilation error: for v in values { spawn(|| { // Do stuff with v }); } Error output as follows: error[E0373]: closure may outlive the current function, but it borrows `v`, which is owned by the current function --> <anon>:7:15 | 7 | spawn(|| { | ^^ may outlive borrowed value `v` 8 | v; | - `v` is borrowed here | help: to force the closure to take ownership of `v` (and any other referenced variables), use the `move` keyword, as shown: | spawn(move || { You can play with the code here: https://is.gd/f4guCw https://is.gd/f4guCw As the error message says, the real problem with this code, given Rust's semantics, is that the closures are being given pointers to memory that might not be valid by the time the closure gets around to executing (unlike in Go (or any other GC'd lang), the mere existence of a pointer is not enough to keep memory alive). And as the help text at the bottom of the error message describes, one solution is to have the closures themselves assume ownership of the data via the `move` keyword on closures.
  - lobster_johnson 10y ago
    Excellent, thanks. Great error message.
    - isaacremuant 10y ago
      Go might have some complex concepts initially and be a big language but its error messages are impressively helpful and precise. I'm very thankful to that level of dedication.
      - kibwen 10y ago
        The error message above was from Rust, not Go, is that what you meant? :P
- eximius 10y ago
  Rust will prevent you from doing this, mostly. Basically, the borrow checker will either force you to move the value, preventing it from being used elsewhere, or copy/clone the value which prevents any issues.
eridius 10y ago
I had a relatively simple CLI tool written in Go that nevertheless had a data race bug that I hit about once a month. I wasn't able to track it down until Go actually introduced a data race detector, at which point I found it immediately. The race occurred when I spawned a task and watched the task's stdout and stderr. I assumed it used one goroutine that listened on both pipes, but it turned out that the stdlib actually used one goroutine per pipe, meaning the shared buffer that I'd captured in both callback functions was being raced on. Of course, the stdlib didn't document how many goroutines it used in that scenario. So yeah, footgun, meet foot.
Manishearth 10y ago
I was going to leave basically this same comment -- I like Go, but the fact that it doesn't have nice locking support always gets to me. (Almost every Go program I've written uses goroutines so I end up using locks in almost every program I write) There's no way to do this without language support or sacrificing perf with interface, so I get why nicer locks don't exist, I just wish they did.

atombender 10y ago

More than once I've also had to kick myself for making the rookie mistake of closing over a loop variable. So something like: for _, v := range values { go func() { // Do stuff with v } } Instead of: for _, v := range values { go (func(v string) { // Do stuff with value })(v) } It's rare enough, and subtle enough, that every instance tends to result in 5-10 minutes of puzzled debugging with stdout printing statements until I realize what's going on. What does Rust do here? I like to say that Go's strictness is unevenly distributed. Unused imports are illegal, but Go is perfectly happy to let you shadow variables, have closure use loop variables, or reassign the built-in values ("nil", "true", etc.). It's like a parent who locks the scissors away in a drawer but doesn't mind leaving drain cleaner on the kitchen table.

Jabbles 10y ago
You should use go vet https://golang.org/cmd/vet/#hdr-Struct_tags https://golang.org/cmd/vet/#hdr-Struct_tags (There seems to be a documentation error.) And try the race detector: https://golang.org/doc/articles/race_detector.html https://golang.org/doc/articles/race_detector.html Though of course, it's hard to argue that it would be nice to prevent these from compiling.
- dispose13432 10y ago
  Well, that's an "easy" bug. What about not-shadowing a "non-loop" variable? Take a look at this question: http://stackoverflow.com/questions/18499352/golang-cuncurrency-how-to-append-to-the-same-slice-from-different-goroutines http://stackoverflow.com/questions/18499352/golang-cuncurren... It's easy to mess up during refactoring, and there's pretty much no reason to allow it.
- lobster_johnson 10y ago
  Indeed, it's invaluable. I use "go vet" and have enabled all of the Gometalinter linters [1] that make sense. "go vet" never warned me about goroutine closure errors, not sure why, maybe I was running an old version. But I'm glad that it's supported. That said, some of the things that "go vet" catches should, in my opinion, be errors. [1] https://github.com/alecthomas/gometalinter https://github.com/alecthomas/gometalinter
dilap 10y ago
I feel like this specific issue Go just got wrong. 99% of the time you want a new variable in the range loop. I think Go's strictness is very practical -- it's strict where they saw real problems and the cure was easy. Something like threading race conditions is a real problem, but not easy to fix. Go does give you a pretty good runtime tool with the race checker, though. It would quickly catch something like the fish bug of grabbing the wrong lock.
- dispose13432 10y ago
  >Something like threading race conditions is a real problem, but not easy to fix. It is. It's called channels.
  - dilap 10y ago
    Sure, you could only use channels and you'd never get races, but in practice that'd be unwieldy and slow, which is why people write traditional lock-based sync stuff in Go all the time, and depend on old-fashioned debugging (and the race-checker). Rust really fixes this (with its ownership system), but it wasn't easy.
    - dispose13432 10y ago
      >unwieldy and slow So force one to do it consciously (kind of like unsafe).
    - masklinn 10y ago
      > Sure, you could only use channels and you'd never get races Go allows sending pointers to non-locked structures over channels, so it's quite easy to "only use channels" and still get race. You can also hit that issue if you spawn multiple goroutines sharing the same initial lexical environment if they make use of a mutable structure from that environment.
      - dilap 10y ago
        If you're accessing mutable data from multiple goroutines, then you're not only using channels! :)
Animats 10y ago
Shadowing a variable really ought to be an error. At least, you shouldn't be able to hide a local variable with another local variable. Yes, sometimes you have to write "vv" in the inner loop instead of "v", and not feel as l33t. Deal with it. (A shadow variable problem in C just turned up in firmware for a surface mount reflow soldering oven I have. The variable name is "avgtemp". This may explain why some ovens scorch PC boards.[1] Read the discussion. Note that none of the people writing about the issue understand that "static" would confine the scope to one file, and that uninitialized variable declarations at top level are global to the whole program. That's probably because they came up from Arduino land, where people are not taught to think about that stuff. Arduino land is really full C++ using gcc, but it's not taught that way.) [1] https://github.com/UnifiedEngineering/T-962-improvements/issues/90 https://github.com/UnifiedEngineering/T-962-improvements/iss...
- jjnoakes 10y ago
  I don't think the parent is talking about problems with shadowing.
  - Animats 10y ago
    They wrote "but Go is perfectly happy to let you shadow variables...", although their main issue was with closures.
    - jjnoakes 10y ago
      Whoops, right you are!
- vram22 10y ago
  >Note that none of the people writing about the issue understand that "static" would confine the scope to one file That seems a bit surprising, unless they did not know C even reasonably well. IIRC (though I haven't used C much lately, I did a lot with it earlier), that is a not-too-advanced feature of the C language. I think it is covered in the K&R C book, near the middle or in the latter half (don't have it handy right now to check).
electrum 10y ago
Fortunately, that mistake isn't possible in Java with lambda expressions or anonymous classes. It is a compile error if any captured variables are not "effectively final". Prior to Java 8, which introduced lambdas, variables used with anonymous classes were required to be final. That restriction is now relaxed as the compiler can infer it. Interestingly, the for-each loop variable is effectively final, unless you explicitly modify it, so this code is legal (and correct): for (String v : values) { executor.execute(() -> System.out.println(v)); }
- sgift 10y ago
  I can never decide if enforcing (effectively) final is a good feature or a bad one. Sure, you cannot shoot yourself in the foot with these limits, on the other hand you loose all the power of real closures.
  - twblalock 10y ago
    You can easily modify variables outside of the stream. For example, to increment a counter declared outside of the lambda, use a final AtomicInteger or similar class instead of an int. For any other value, use a final class that wraps it. The point of enforcing final variables is to prevent programmers from accidentally modifying things they don't want to. It does not prevent programmers from modifying variables intentionally.
    - sgift 10y ago
      I know very well that I can use inner mutability to work around that restriction, that doesn't change the fact that it is a workaround, nothing more.
      - twblalock 10y ago
        It's not a workaround. It was designed to work that way on purpose. The people who developed this feature could have easily prevented programmers from using any variables outside of the stream, final or not, but they chose not to because they wanted to allow programmers to use final variables in this way. Furthermore, it does not cause you to lose "all the power of real closures" like you said it does. All you lose is the ability to use a closure around non-final variables, which is a trivial drawback in any use case I've ever come across. You lose only a little bit of the power of closures.
tatterdemalion 10y ago
I don't know Go, so I don't really understand what the issue is with the code you posted. In Rust, the closure used to spawn a new thread needs to own its environment; if it does then there's no problem. If it doesn't that's a data race and you have a compile time error about it.
- lobster_johnson 10y ago
  The error in the Go snippet is that "v" is a single memory location shared throughout the loop. The closure doesn't get a copy, so what usually ends up happening is that every goroutine gets the last value (since the goroutines are probably not scheduled until the end of the loop). This problem doesn't just affect loops, of course: A goroutine can access any local environment outside its scope, and local variables can mutate. The worst surprise I ever encountered was this (simplified): func makeWorkersDocomplicatedStuff() ch := make(chan string) defer func() { if ch != nil { close(ch) } } for i := 0; i < numWorkers; i++ { go func() { for { select { case s := <-ch: // ... } } } } close(ch) ch = nil // ... More stuff ... } What will happen here is that the goroutines will all block forever, because "ch" becomes nil, and in Go, polling a nil channel will block forever (it's an interesting design choice in such a strict language). Rewriting and refactoring this was trivial enough, but catching it was time wasted. Lessons learned: (1) Be scrupulous about closure environments, (2) be super careful about nil channels, and (3) try to avoid defers whose concerns don't fully encapsulate the function body (so defers in the middle of a function is often code smell).
  - tatterdemalion 10y ago
    > The error in the Go snippet is that "v" is a single memory location shared throughout the loop. In Rust this is not true. However, the same concept applies - say it was a variable from outside the loop. In that case, you would get a clear compile time error about moving the value into the thread's closure more than once. A similar error would apply if you iterated over a container by reference (instead of by value).
kibwen 10y ago
> What does Rust do here? This code results in a compilation error: for v in values { spawn(|| { // Do stuff with v }); } Error output as follows: error[E0373]: closure may outlive the current function, but it borrows `v`, which is owned by the current function --> <anon>:7:15 | 7 | spawn(|| { | ^^ may outlive borrowed value `v` 8 | v; | - `v` is borrowed here | help: to force the closure to take ownership of `v` (and any other referenced variables), use the `move` keyword, as shown: | spawn(move || { You can play with the code here: https://is.gd/f4guCw https://is.gd/f4guCw As the error message says, the real problem with this code, given Rust's semantics, is that the closures are being given pointers to memory that might not be valid by the time the closure gets around to executing (unlike in Go (or any other GC'd lang), the mere existence of a pointer is not enough to keep memory alive). And as the help text at the bottom of the error message describes, one solution is to have the closures themselves assume ownership of the data via the `move` keyword on closures.
- lobster_johnson 10y ago
  Excellent, thanks. Great error message.
  - isaacremuant 10y ago
    Go might have some complex concepts initially and be a big language but its error messages are impressively helpful and precise. I'm very thankful to that level of dedication.
    - kibwen 10y ago
      The error message above was from Rust, not Go, is that what you meant? :P
eximius 10y ago
Rust will prevent you from doing this, mostly. Basically, the borrow checker will either force you to move the value, preventing it from being used elsewhere, or copy/clone the value which prevents any issues.

eridius 10y ago

I had a relatively simple CLI tool written in Go that nevertheless had a data race bug that I hit about once a month. I wasn't able to track it down until Go actually introduced a data race detector, at which point I found it immediately. The race occurred when I spawned a task and watched the task's stdout and stderr. I assumed it used one goroutine that listened on both pipes, but it turned out that the stdlib actually used one goroutine per pipe, meaning the shared buffer that I'd captured in both callback functions was being raced on. Of course, the stdlib didn't document how many goroutines it used in that scenario. So yeah, footgun, meet foot.

Manishearth 10y ago

I was going to leave basically this same comment -- I like Go, but the fact that it doesn't have nice locking support always gets to me. (Almost every Go program I've written uses goroutines so I end up using locks in almost every program I write) There's no way to do this without language support or sacrificing perf with interface, so I get why nicer locks don't exist, I just wish they did.