solver: use errors.Is when checking context.Cause() #4587
Conversation
Since the change to replace uses of context.WithCancel with WithCancelCause, we've also begun wrapping all cancellations using errors.WithStack. This means that these would not directly match context.Canceled, so we need to make sure to use errors.Is. Signed-off-by: Justin Chadwell <me@jedevc.com>
💯
I don't think |
| if err != nil && context.Cause(ctx) == context.Canceled { | ||
| if err != nil && errors.Is(context.Cause(ctx), context.Canceled) { |
There was a problem hiding this comment.
We're only interested in cancelled contexts, here, correct? (as they commonly go hand-in-hand with DeadlineExceeded); e.g. moby/moby#46444
|
It wouldn't surprise me if |
|
Very orthogonal, but may be of interest here as well; there's some work in progress on moving containerd's |
Had a quick check through the options on https://revive.run/docs#available-rules, doesn't appear that there's one that ticks the box for us. |
Since the change to replace uses of
context.WithCancelwithWithCancelCausein #4457, we've also begun wrapping all cancellations usingerrors.WithStack. This means that these would not directly match context.Canceled, so we need to make sure to useerrors.Is.I spotted this while reading over #4457 in more detail again, I wonder if this is the source of a particular deadlock we're encountering where gRPC streams aren't shutting down (though this may or may not be the actual root cause lol). That said, this code definitely seems wrong.
Ideally, we should have some linter check to prevent comparing errors like this at all - direct comparisons on errors should be the exception rather than the rule. cc @crazy-max do you know if this exists?