hashicorp / hashicorp/go-multierror
`error` nil check weirdness with `Group.Wait`
- Dominant language
- Go
- Stars
- 2.6k
- Forks
- 143
- PR merge metrics
- No merged PRs in 30d
Description
It's not uncommon to see code like this:
```go
package main
import (
"github.com/hashicorp/go-multierror"
)
type server struct{}
func (s *server) Run() error {
g := new(multierror.Group)
g.Go(
func() error {
// ...code to do some work here...
return nil
},
)
g.Go(
func() error {
// ...code to do some MORE work here...
return nil
},
)
return g.Wait()
}
func main() {
s := server{}
if err := s.Run(); err != nil {
panic("error while running")
}
}
```
However, there is a subtle bug here. The `main` function will _always panic_, despite no error being returned by any of the Goroutines managed by the group `g`. This is due to the fact that Go treats interfaces as "fat pointers". See:
* https://golang.org/doc/faq#nil_error
* https://tour.golang.org/methods/12
* https://guihao-liang.github.io/2020/07/05/interface-type-value
* https://glucn.medium.com/golang-an-interface-holding-a-nil-value-is-not-nil-bb151f472cc7
* https://bluxte.net/musings/2018/04/10/go-good-bad-ugly/#nil-interface-values
A "fix" is to add the following nil check to the `Run` method above:
```go
func (s *server) Run() error {
g := new(multierror.Group)
g.Go(
func() error {
// ...code to do some work here...
return nil
},
)
g.Go(
func() error {
// ...code to do some MORE work here...
return nil
},
)
if err := g.Wait(); err != nil {
return err
}
return nil
//... or alternatively: return g.Wait().ErrorOrNil()
}
```
This is certainly not intuitive, and relies on type-inference to solve the problem. I've added a test to my fork of this repo to clearly demonstrate the issue:
* https://github.com/ccampo133/go-multierror/pull/1/files
My question - is there a good reason why `Group.Wait` returns `*Error` instead of just `error`, which is more idiomatic? Returning `error` would eliminate this weirdness seen here.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.