gps: use golang.org/x/sync/errgroup #1155
Conversation
|
heh - i realize we've got these scattered around elsewhere, but before merging this, can someone clarify a bit for me on whether it's a design goal of pkg/errors that errors should be wrapped multiple times as they're passed back up the stack, and/or where the "optimal" point to wrap errs in a stack is supposed to be? |
|
Yes, multiple levels of wrapping is the intention, so that you can reconstruct all the handlers of the error. Note that |
|
You're right, we should be using WithMessage when the error already has a stacktrace. |
|
Eh, not true. If you print stack traces (%+v), you'll get them all.
…On Mon, Sep 11, 2017 at 11:10 AM, Jordan Krage ***@***.***> wrote:
You're right, we should be using WithMessage
<https://godoc.org/github.com/pkg/errors#WithMessage> when the error
already has a stacktrace.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPMK3aUzsitm-OpGtEy8qUcDzEePRks5shU2CgaJpZM4PSiJ7>
.
|
| ident: mb.getURL(), | ||
| err: err, | ||
| }) | ||
| errs = append(errs, errors.Wrapf(err, "failed to set up %q", mb.getURL())) |
There was a problem hiding this comment.
errors.WithMessage is cheaper in cases like this too, when we don't need the stack trace.
There was a problem hiding this comment.
I'm not going to make that optimization; it doesn't matter and will be net-negative the very first time someone looks to find where this error originates.
There was a problem hiding this comment.
Doesn't the same logic apply to removing sourceFailures?
There was a problem hiding this comment.
How? sourceFailures provides nothing at all.
There was a problem hiding this comment.
Before this change the final cause in the chain of errors was a sourceFailures with all of its errors preserved, so fiddling with the error at the top level, or with the format verb, could potentially provide some insight.
Now it's an errors.New() with only the string messages of the original errors, so they'll have to come here to edit this method directly to follow the trail any deeper.
An improved version of sourceFailures might implement fmt.Formatter (or be generalized into a multiError{causes, msg}), but I think it's still offering something as-is.
True, that may be useful in some cases. I was particularly thinking about cases like unwrapVcsError though. |
tamird
changed the title
|
@jmank88 alright well, you convinced me. I've rewritten that area of the code since the error handling there bothered me O.o |
|
eek. could you do a |
|
Done. |
internal/gps/result.go
Outdated
| p := lps[resp.i] | ||
| logger.Printf("(%d/%d) %s %s@%s\n", cnt, len(lps), msg, p.Ident(), p.Version()) | ||
| } | ||
| logger.Printf("(%d/%d) %s %s@%s\n", atomic.AddUint32(&cnt, 1), len(lps), msg, p.Ident(), p.Version()) |
There was a problem hiding this comment.
Are these technically racing to log after the atomic add (meaning they could reorder)? Would the op have to be crammed inside a fmt.Formatter to get around that?
There was a problem hiding this comment.
Yeah, they might reorder. I'm not sure fmt.Formatter would do it, because there's still a race in the machinery that ends up calling Write with the result.
We could do it under our own lock, though.
There was a problem hiding this comment.
What about moving the sem stuff into the body of err func()? Then another lock here is not such a big deal.
There was a problem hiding this comment.
I don't follow. What does the semaphore have to do with anything?
There was a problem hiding this comment.
Then another worker is freed before we potentially lock for this count, or even now before the logger locks internally.
|
Eh, I don't think that matters, this workload is dominated by I/O.
…On Mon, Sep 11, 2017 at 11:20 PM, Jordan Krage ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In internal/gps/result.go
<#1155 (comment)>:
> }
- errs = append(errs, resp.err)
- msg = "Failed to write"
- }
- p := lps[resp.i]
- logger.Printf("(%d/%d) %s %s@%s\n", cnt, len(lps), msg, p.Ident(), p.Version())
- }
+ logger.Printf("(%d/%d) %s %s@%s\n", atomic.AddUint32(&cnt, 1), len(lps), msg, p.Ident(), p.Version())
Then another worker is freed *before* we potentially lock for this count,
or even now before the logger locks internally.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPNN8sj6_CJHeD2uQFGJj9fT4dusEks5shfiCgaJpZM4PSiJ7>
.
|
|
Hopefully wiring through that |
|
Should be fixed now.
…On Mon, Sep 11, 2017 at 11:38 PM, Jordan Krage ***@***.***> wrote:
errgroup is neat. I think we'll find a few other places to swap it right
in.
Hopefully wiring through that Context to fix the build isn't too deep a
rabbit hole
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPGUYNL1qWvK9QEUHxIJx9coB2l5iks5shfzAgaJpZM4PSiJ7>
.
|
|
Urgh, validate-vendor doesn't like pruning. Fixed. |
|
Also added a mutex as we discussed. |
yeah, i was just fixing this locally, actually. maybe hold a sec on it |
|
that'll be #1161 |
|
should be a pretty straightforward rebase now, though probably not entirely conflict-free |
...instead of custom code that does the same thing. Return
informative errors rather than logging directly (into the abyss).
Also remove sourceFailure{,s} while I'm here, which are unnecessary.
Also `dep prune` real quick.
|
Done.
…On Tue, Sep 12, 2017 at 1:44 PM, sam boyer ***@***.***> wrote:
should be a pretty straightforward rebase now
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPCkS7ZVwIoMCUBzDDdQAqz39Zbxrks5shsLwgaJpZM4PSiJ7>
.
|
|
Green. |
|
codeclimate is grumpy because I added a context.TODO() but I looked up the stack and didn't see an opportunity to plumb an existing one. I'll look again. |
|
i can't recall if there's an opportunity for one higher up, but it's fair to say that in general our wiring of |
|
gonna do a proper grok & review this evening |
|
Yeah, plumbing the context is a deep rabbit hole. We end up in
cmd/dep/ensure.go with 7 callers, none of which have a context to pass.
…On Tue, Sep 12, 2017 at 3:51 PM, sam boyer ***@***.***> wrote:
gonna do a proper grok & review this evening
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPE7Ilhml6H8-8io27qr69nUlcUC6ks5shuDBgaJpZM4PSiJ7>
.
|
|
(So I'm going to leave it as-is) |
|
yepyepyepyep 🎉 🎉 |
...instead of custom code that does the same thing. Return
informative errors rather than logging directly (into the abyss).
Also remove sourceFailure{,s} while I'm here, which are unnecessary.
What does this do / why do we need it?
Includes more information in errors which bubble up in tests, for one.
What should your reviewer look out for in this PR?
Not much, pretty straightforward.
Do you need help or clarification on anything?
No.
Which issue(s) does this PR fix?
None.