Skip to content

lib/errors: unify multi-errors to fix printing, update error dependencies

Warren Gifford requested to merge errors-fix into main

Created by: bobheadxi

The issue

image

Someone should formulate a plan to deprecate use of lib/errors.Append and use lib/errors.Combine instead. The issue here is that they’re not semantically equivalent: the former uses a bag of errors and the later has a primary and secondary. I think we need to tag each of the former with a banner error and use the bag as the secondary error. This might take some amount of manual vetting, there’s a number of references that exist.

Seemed simple enough...

Tangent

I initially started with a spike to explore removing go-multierror entirely. This revealed a few things:

  • If anyone expects cockroach's CombineErrors to allow them to introspect on the underlying error via Is, Unwrap, As and friends, well you can't! https://github.com/cockroachdb/errors/issues/62 The underlying type, SecondaryError, completely obscures the second error, except in certain printing situations.
    • This means that my (and maybe others') assumption that CombineErrors is like Append but for 2 errors instead of vararg errors is not true, the two are fundamentally different
    • You can use Wrap, but it doesn't allow you to wrap error A within error B, so deceptively CombineErrors seems to be the solution when it actually probably isn't
    • A test reveals that CombineErrors doesn't show the second error in .Error(), %s, and possibly more so the issue raised about go-multierror actually applies to all our methods of error combination
  • I attempted to rewrite our own multi error type, but it turns out Unwrap doesn't really work the way I think it does with Cockroach errors. For example:
reference := New("foo")
unwrapped := Unwrap(err)
assert.NotNil(t, err) // okay, so there's an underlying error to a raw "New" error...?
assert.True(t, Is(unwrapped, reference)) // fails! unwrapped error no longer asserts against a sentinel reference error
assert.True(t, Is(reference, unwrapped)) // well this works but semantically makes no sense because the second argument is supposed to be the reference

New comes with a stack layer that gets unwrapped on Unwrap, which makes using New errors as sentinel errors dangerous - if you unwrap an error too far the sentinel error just doesn't work anymore! So we can't depend on the behaviour of Unwrap to build a lightweight multierror layer on top of cockroach errors, and now I'm also unsure if I can depend on Unwrap to do anything

edit so it turns out I was doing it wrong, see https://github.com/sourcegraph/sourcegraph/pull/31466

Attempted solution

(this PR)

Attempts to bandaid-resolve the issue raised today at backend-crew by patching go-multierror to fix the main issue of "printing doesn't show all errors in multierrors". See: https://github.com/hashicorp/go-multierror/compare/master...sourcegraph:master

Migrates CombineErrors to proxy to go-multierror instead (see below)

Removes unnecessary ErrorListFunc re-export

The better solution?

https://github.com/sourcegraph/sourcegraph/pull/31466

Test plan

Unit tests

Merge request reports

Loading