8000 Add Context by nzoschke · Pull Request #8 · bradleyfalzon/ghinstallation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add Context #8

8000
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add Context #8

wants to merge 1 commit into from

Conversation

nzoschke
Copy link
@nzoschke nzoschke commented Feb 4, 2018 •
edited
Loading

This patch adds the context pattern to the package. The transports have a Context property which defaults to context.Background() in the New* methods.

The context pattern facilitates managing timeouts and adding tracing to a system.

To use it with AWS XRay do:

ctx, seg := xray.BeginFacadeSegment(context.Background(), "name", header.FromString(os.Getenv("_X_AMZN_TRACE_ID")))
xc := xray.Client(nil)
t, err := ghinstallation.NewKeyFromFile(xc.Transport, integrationID, installationID, "key.pem")
if err != nil {
	return nil
}
t.Context = ctx

See google/go-github#526 for a good discussion about the context pattern in the GitHub client.

@nzoschke
Copy link
Author
nzoschke commented Feb 4, 2018

Here's evidence of this patch facilitating tracing with AWS Xray.

screen shot 2018-02-04 at 8 17 49 am

@bradleyfalzon
Copy link
Owner

Thanks for the PR and I do see value in it, my only concern is the breaking change.

Could we propose to export a Transport.Ctx, in a similar way to how we do this with the Transport.Client? Defaulting to context.Background() in the New* methods?

Also, we're now importing another library, that brings in questionable value, it seems like we could achieve the same thing without another dependency because our requirements are pretty light.

I'll need to look a this in a little bit more detail, but that's my initial thoughts. Happy to be corrected/challenged if you've already thought them through.

dmitshur
dmitshur reviewed Feb 5, 2018
View reviewed changes
Copy link
@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions. It seems a little unexpected to me that contexts are stored as part of the AppsTransport and Transport structs. I'm not very familiar with the API or usage of this package.

appsTransport.go Outdated
@@ -75,6 +79,7 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) {
req.Header.Set("Authorization", "Bearer "+ss) 8000
req.Header.Set("Accept", acceptHeader)

resp, err := t.tr.RoundTrip(req)
c := t.Client.(*http.Client)
resp, err := ctxhttp.Do(t.ctx, c, req)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why the RoundTrip call is being replaced by a Do?

This seems suspicious to me, because Do implements higher-level behavior than RoundTrip. Notably, https://godoc.org/net/http#RoundTripper says:

RoundTrip executes a single HTTP transaction, returning
a Response for the provided Request.

RoundTrip should not attempt to interpret the response. In
particular, RoundTrip must return err == nil if it obtained
a response, regardless of the response's HTTP status code. ...

Whereas Do's behavior is to follow redirects, etc.

appsTransport.go Outdated
@@ -43,12 +46,13 @@ func NewAppsTransportKeyFromFile(tr http.RoundTripper, integrationID int, privat
// installations to ensure reuse of underlying TCP connections.
//
// The returned Transport's RoundTrip method is safe to be used concurrently.
func NewAppsTransport(tr http.RoundTripper, integrationID int, privateKey []byte) (*AppsTransport, error) {
func NewAppsTransport(ctx context.Context, tr http.RoundTripper, integrationID int, privateKey []byte) (*AppsTransport, error) {
Copy link
@dmitshur dmitshur Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contexts are usually request-scoped. Is NewAppsTransport really called once to process every request? From its name, I would imagine it's called once at init time, and then the AppsTransport's RoundTrip method is used per request. Is that not how this works?

transport.go Outdated
@@ -33,6 +36,7 @@ type Transport struct {

mu *sync.Mutex // mu protects token
token *accessToken // token is the installation's access token
ctx context.Context
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx is not protected by mu, is it? Putting it here as you did makes it seem like it is.

See https://dmitri.shuralyov.com/idiomatic-go#mutex-hat.

@dmitshur
Copy link
dmitshur commented Feb 5, 2018 •
edited
Loading

I also have the following comments regarding @bradleyfalzon's comment.

my only concern is the breaking change. Could we propose to export a Transport.Ctx, in a similar way to how we do this with the Transport.Client? Defaulting to context.Background() in the New* methods?

Given the breaking changes the go-github library sees relatively often, including google/go-github#529 itself, I would imagine a breaking change, if needed, to add context.Context support would not be very objectionable. There are only 3 known public importers of this package, so updating code shouldn't be expensive.

I think a breaking change with a clean API is a better decision than no breaking change the cost of a suboptimal API. (That said, I don't know what the optimal API is.)

@nzoschke
Copy link
Author
nzoschke commented Feb 5, 2018

Great feedback. I have these questions myself.

I think exporting Context and having a default will work just fine to not change the API.

I think we have to store the context on the struct since there isn’t an explicit API for performing requests that would take s context argument.

RoundTrip is replaced with a Do because the ctxhttp package is the standard way of performing context aware requests.

It should still wrap and use the RoundTrip but I’m not sure if there are other behavior side effects.

I can look into copying the ctxhttp.Do method if the extra dependency is unwanted.

@nzoschke
Copy link
Author
nzoschke commented Feb 8, 2018

I replaced my original PR with one that does not change the API.

It also doesn't bring in an additional dependency, however it does not emulate the additional error handling here: https://github.com/golang/net/blob/master/context/ctxhttp/ctxhttp.go#L31

@nzoschke
Copy link
Author
nzoschke commented Feb 8, 2018

Hm, this new patch isn't actually working yet, i'm getting a panic from the xray client just like when no context is passed in.

@mspiegel mspiegel mentioned this pull request May 8, 2018
Closed
@wlynch wlynch mentioned this pull request Jul 15, 2019
Merged
@bradleyfalzon
Copy link
Owner

Considering #21 was merged, can this one be closed?

@bradleyfalzon
Copy link
Owner

Closing as I don't believe this is required anymore. Happy to reconsider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitshur dmitshur dmitshur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nzoschke @bradleyfalzon @dmitshur
0