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.

Sign up for GitHub

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

Jump to bottom

error nil check weirdness with Group.Wait #57

Open
ccampo133 opened this issue Nov 11, 2021 · 8 comments · May be fixed by #58
Open

error nil check weirdness with Group.Wait #57

ccampo133 opened this issue Nov 11, 2021 · 8 comments · May be fixed by #58

Comments

@ccampo133
Copy link

ccampo133 commented Nov 11, 2021 •
edited

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:

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:

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
Copy link

bfreis commented Nov 11, 2021

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
Copy link

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

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

@ccampo133
Copy link
Author

@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.

@ccampo133 ccampo133 changed the title Nil check weirdness with Group.Wait error nil check weirdness with Group.Wait Nov 12, 2021
@purpleidea
Copy link

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 added a commit to ccampo133/go-multierror that referenced this issue Nov 12, 2021
@ccampo133
Return error from Group.Wait
f5409ce 
Changes the return value of Group.Wait to error instead of *Error. The *Error concrete type can lead to unintuitive, subtle bugs around nil checks (see: https://golang.org/doc/faq#nil_error). Returning the error interface instead eliminates these. Addresses hashicorp#57.
@ccampo133 ccampo133 linked a pull request Nov 12, 2021 that will close this issue
Open
@ccampo133
Copy link
Author

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

ccampo133 added a commit to ccampo133/go-multierror that referenced this issue Nov 12, 2021
@ccampo133
Return error from Group.Wait
7d7a61a 
Changes the return value of Group.Wait to error instead of *Error. The *Error concrete type can lead to unintuitive, subtle bugs around nil checks (see: https://golang.org/doc/faq#nil_error). Returning the error interface instead eliminates these. Addresses hashicorp#57.
@chrisguiney
Copy link

chrisguiney commented Aug 31, 2022 •
edited

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
Copy link

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
Copy link
Author

ccampo133 commented Oct 17, 2022 •
edited

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use error instead of *Error in Group ccampo133/go-multierror
5 participants
@purpleidea @chrisguiney @bfreis @ccampo133 @stephanwesten