Improved request errors

[biotom]

Description

Errors returned in the file "Requests.go" were wrapped with useful error messages, and defer statements were added to http requests. Some extra error handling was added to "Requests_test.go".

Motivation and Context

Requests.go returns unwrapped errors (often from common 3rd party libraries), so adding wrapping to those errors might provide more context and make troubleshooting easier. Additionally,
deferred calls to Close() were added after http requests to prevent resource leakage. Require.Noerror statements were added to requests_test.go to handle unhandled errors.

How Has This Been Tested?

Existing unit tests passed, and the code shouldn't cause changes to public behaviour. Error messages are wrapped to provide context, but the testing suite does not rely on literal matching of error messages.

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

[JoelSpeed]

LGTM, would you mind adding a note to the changelog and then we can get that merged 👍

[loshz]

Couple of formatting changes.

[loshz]

While I agree with making these errors more verbose is a good idea, what advantage do we get over just doing:

fmt.Errorf("something bad happened: %v", err)

Am I right in thinking the callers of these functions would then need to parse the "new" errors to check for Cause()?

[loshz]

We could also be super fancy and start to implement some of the new go1.13 error functions https://blog.golang.org/go1.13-errors

[biotom]

While I agree with making these errors more verbose is a good idea, what advantage do we get over just doing:

fmt.Errorf("something bad happened: %v", err)

Am I right in thinking the callers of these functions would then need to parse the "new" errors to check for Cause()?

For me, the big advantage of Wrap is that you're automatically putting together the same sort of useful error string that you'd get with Errorf, but you can easily string a bunch of them together, making it easier to work out what went wrong for debugging. You also get a stack trace from the point at which Wrap was called. This extra context is useful when you're dealing with errors from 3rd party libraries, which might return a generic and unhelpful error message. You'd want to call Cause() further up the error chain if you want to do something based on what the original error was- but in the spirit of using opaque errors, and handling them based on behaviour, I haven't used it very often.

I know error handling in Go is a bit of a can of worms, so I could be wrong about all this...

[biotom]

Apologies- I'm not really sure what I've broken here. Travis is failing with "go: golang.org/x/oauth2@v0.0.0-20190604053449-0f29369cfe45: unrecognized import path "golang.org/x/oauth2" (https fetch: Get https://golang.org/x/oauth2?go-get=1: dial tcp 74.125.69.141:443: i/o timeout)", but go mod tidy adds it to go.sum again when it is removed.

[loshz]

so I could be wrong about all this...

You are definitely not wrong! I think this PR and your feedback is extremely useful.

My only "concern" is that http://github.com/pkg/errors was just a stop gap until we had better error handling in the Go std lib. For example, their roadmap says they won't cut anymore releases after v1.0

1.0. Final release.

As of Go 1.13, there is a new Unwrap() func in the std lib errors package. Here is a playground with an example.

My thoughts are, we could start implementing this new functionality instead of importing a new 3rd party package. WDYT @biotom @JoelSpeed?

[biotom]

My thoughts are, we could start implementing this new functionality instead of importing a new 3rd party package. WDYT @biotom @JoelSpeed?

I think this makes a lot of sense! It looks to me like the new error handling is mostly the best wrapping bits from the errors library at any rate- I'd be happy to look into it, if people feel like that's a good idea.

[loshz]

I'd be happy to implement this new functionality with you if you want to work on it together? I can create an issue which outlines our intentions and a list of changes we need to make.

[biotom]

I'd be happy to implement this new functionality with you if you want to work on it together? I can create an issue which outlines our intentions and a list of changed we need to make.

Sounds great!

[JoelSpeed]

As of Go 1.13, there is a new Unwrap() func in the std lib errors package. Here is a playground with an example.

Out of interest, if this is in 1.13, can we not just use 1.13? 🤔

[loshz]

Out of interest, if this is in 1.13, can we not just use 1.13? 🤔

@JoelSpeed I'm not sure what you mean by this?

There is now a new %w verb that can be passed to fmt.Errorf which automatically wraps errors, so we would need to implement this.

https://golang.org/src/fmt/errors.go

[loshz]

Couple of small nitpicks.

We will also need to drop support for Go 1.12 in .travis.yml. Removing this line will do the trick.

[loshz]

Thank you for you patience and input on this PR! Congrats on your first contribution 🎉

[biotom]

Thank you for you patience and input on this PR! Congrats on your first contribution 🎉

Thanks! And thanks for all the support with my first open source contribution! I'd be happy to help check out error handling in the rest of the code, if you'd still be interested.

[loshz]

@biotom I plan on creating an issue for a general refactor of errors as mentioned above. I'll add you as an assignee and you can help out whenever suits you best 😄

Also, we have an #oauth2_proxy channel in Gophers Slack if you'd like to get involved in other conversations.