error nil check weirdness with Group.Wait

[ccampo133]

It's not uncommon to see code like this:

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:

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.

[bfreis]

By obtaining an *Error, you can do useful stuff like retrieve the WrappedErrors(). Also, check the method ErrorOrNil(), which seems to have been designed exactly to avoid the problem you encountered.

[stephanwesten]

See https://golang.org/doc/faq#nil_error

still, i don’t get the need to return a pointer, just return a struct.

[ccampo133]

@bfreis if Wait returned error instead of *Error, you could still retrieve an *Error with a type assertion or using errors.As. For example:

err := g.Wait().(*multierror.Error)
errs := err.WrappedErrors()
...

or

var e *multierror.Error
if errors.As(g.Wait(), &e) {
	// err is a *multierror.Error, and e is set to the error's value
	errs := e.WrappedErrors()
	...
}

From that FAQ link above (thanks @stephanwesten), the Go maintainers recommend:

It's a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly. As an example, os.Open returns an error even though, if not nil, it's always of concrete type *os.PathError.

Seems like that would be a good pattern to follow here as well. It would certainly eliminate this sort of gotcha, which in my opinion, is quite an ugly wart of Go itself.

[purpleidea]

It's just a bug in the library:

https://github.com/hashicorp/go-multierror/blob/master/group.go#L33

func (g *Group) Wait() *Error {

The signature should end with error instead. Send a small patch.

Cheers

[ccampo133]

PR #58 addresses this issue by changing to error instead of *Error where necessary. Thanks :)

[chrisguiney]

I think an additional issue here is that Append() will return a non-nil error even when all arguments are nil.

e.g.,

Append(nil, nil) != nil

It should really return

return Append(&Error{}, newErrs...).ErrorOrNil()

edit:
and then Append still returns an *Error, and so it will have a non-nil value if assigned to an error, because the value will have an underlying type.

Append() really should return a error instead of *Error, but I can see that being a big backwards compatibility break

In this case, Group should probably keep the member variable as a *Error, and Wait() error should call g.err.ErrorOrNil() upon return

[purpleidea]

PR #58 addresses this issue by changing to error instead of *Error where necessary. Thanks :)

Not to sound like a total jerk, but I think it's customary to put a thanks @purpleidea or similar in the commit log when someone gives you the answer for your patch =D

[ccampo133]

Seems this repository is dead anyway, considering even this tiny PR has sat nearly a year without as much as a glance.

Also, not to be a total jerk @purpleidea, but did you really provide the answer? I mentioned the provided solution in the description of this issue when I opened it ;).

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.