This seems like a terrible misunderstanding of the errgroup package? The entire point of the returned context is that it's cancelled if an error occurs, yet the author never uses it again until after he's done with the errgroup. The behaviour is also clearly documented in the only function exported by the package.
A subtle bug with Go's errgroup
11–20 of 21 posts
Re: A subtle bug with Go's errgroup
#12This seems like a terrible misunderstanding of the errgroup package? The entire point of the returned context is that it's cancelled if an error occurs, yet the author never uses it again until after he's done with the errgroup. The behaviour is also clearly documented in the only function exported by the package.
> The entire point of the returned context is that it's cancelled if an error occurs, yet the author never uses it again until after he's done with the errgroup.
`Wait()` also does that. And the examples in the package documentation don't show the context as a way for the caller to be notified that things are done (that's what `Wait()` is for) but as a way for the callees (the callbacks passed to Do) to early abort.
This is mostly confirmed by the discussion dantillberg linked above, where someone suggests passing the errgroup's context down to the callbacks as parameter and the package author replies they don't do that because the lack of inference makes for nasty boilerplate (https://github.com/golang/go/issues/34510#issuecomment-53961...).
Re: A subtle bug with Go's errgroup
#13This seems like a terrible misunderstanding of the errgroup package? The entire point of the returned context is that it's cancelled if an error occurs, yet the author never uses it again until after he's done with the errgroup. The behaviour is also clearly documented in the only function exported by the package.
> The entire point of the returned context is that it's cancelled if an error occurs, yet the author never uses it again until after he's done with the errgroup. `Wait()` also does that. And the examples in the package documentation don't show the context as a way for the caller to be notified that things are done (that's what `Wait()` is for) but as a way for the callees (the callbacks passed to Do) to early abort.…
Yeah, just... like, don't use ctx from WithContext after the errgroup. WithContext is an API that specifically and explicitly cancels the context on error. It's absolutely optional. It's not a bug at all. It's using the wrong APIs and using a variable out of intended scope.
Re: A subtle bug with Go's errgroup
#14I love errgroup, though I always use it like this
g, gCtx := errgroup.WithContext(ctx)
That would have avoided the problem.
Perhaps worth a submission to https://staticcheck.dev/ ?
Re: A subtle bug with Go's errgroup
#15So it's really the issue due a partial variable shadowing during assignment, not errgroup-specific. Got bitten by it multiple times unfortunately, but I don't think there's an easy lint or vet check that could prevent such errors from happening. Having said that, overwriting a variable during := assignment is typically only useful for errors, so potentially you might want to have a lint check that complains when you'…
No? Its about errgroup ctx being automatically cancelled once the group is done (in success or err). It kinda makes sense, and its documented. I understand how it can be surprising, but if you think about it for a second, it makes sense. The context created is meant to manage the lifecycle of the errgroup. Once the group is done, the ctx is done.
It's only about context _returned by errgroup_ being cancelled, you shouldn't be overwriting it in the way in which it was used by an article, which is really hard to spot due to variable shadowing.
Re: A subtle bug with Go's errgroup
#16Earlier quoted context omitted.
No? Its about errgroup ctx being automatically cancelled once the group is done (in success or err). It kinda makes sense, and its documented. I understand how it can be surprising, but if you think about it for a second, it makes sense. The context created is meant to manage the lifecycle of the errgroup. Once the group is done, the ctx is done.
It's only about context _returned by errgroup_ being cancelled, you shouldn't be overwriting it in the way in which it was used by an article, which is really hard to spot due to variable shadowing.
> variable shadowing
There is no shadowing.
It's also harder to spot that this is a bad pattern because it's exactly what the package examples currently do.
Re: A subtle bug with Go's errgroup
#17> Shadowing is another concept that made this issue less visible. There is no shadowing here, Go can only shadow in different scopes, `ctx` is just rebound (overwritten). Zig does not prevent reusing variables that I know. However I believe zig parameters are implicitly `const`, which would have prevented thoughtlessly updating the local in-place. Same in Rust. > If you've ever heard of linear types, and never saw th…
Your are right about shadowing: I just checked and the go specification calls it “redeclaration” and it works as you described:
> Redeclaration does not introduce a new variable; it just assigns a new value to the original
I added a mention about that in the article. Thank you.
Re: A subtle bug with Go's errgroup
#18[deleted]
Re: A subtle bug with Go's errgroup
#19I love errgroup, though I always use it like this g, gCtx := errgroup.WithContext(ctx) And then use gCtx in the g.Go function calls. That would have avoided the problem. Perhaps worth a submission to https://staticcheck.dev/ ?
I just made very simpler linter for this: https://github.com/nikolaydubina/lint-errgroup-ctx
Re: A subtle bug with Go's errgroup
#20omg. you are right. found multiple bugs just now thanks to this... they sould really improve this API of waitgroup/errgroup
UPD: made a linter https://github.com/nikolaydubina/lint-errgroup-ctx