hashicorp / hashicorp/go-multierror

`error` nil check weirdness with `Group.Wait`

Open
#57 8 comments 1 reaction 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.