support the context package

[kevinburke]

It would be nice if go-github supported the context package, so you could coordinate cancelation across many different goroutines.

It's now available in the standard library as https://golang.org/pkg/context/.

[dmitshur]

In theory, you can do it, but not directly. You'd have to use Client.NewRequest to create a new *http.Request, then set the context of that HTTP request via Request.WithContext, and finally call github.Client.Do.

However, it's so roundabout that it's probably not even worth considering.

To resolve this fully, we'd have to break the API and add ctx context.Context as first parameter to all methods.

---

To get an idea of what it'd look like, see github.com/tambet/go-asana/asana, a similar package that is the client for Asana API, where context support was added (in tambet/go-asana#4).

---

I'd be in favor of doing that, since it's only way to move forward. People who vendor this library can update on their own time. We would bump libraryVersion.

However, this decision is largely in the hands of @willnorris and @gmlewis.

[dmitshur]

I'd like to work on this, if we can get approval.

Some planning thoughts on how it would be done.

The change would be largely mechanical, once we decide on how to make the change. It would affect all endpoint methods by adding ctx context.Context as the first parameter. For example:

// Get a single issue.
//
// GitHub API docs: http://developer.github.com/v3/issues/#get-a-single-issue
func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Response, error) { ... }

Would become:

// Get a single issue.
//
// GitHub API docs: http://developer.github.com/v3/issues/#get-a-single-issue
func (s *IssuesService) Get(ctx context.Context, owner string, repo string, number int) (*Issue, *Response, error) { ... }

The inside of a typical method looks like this:

func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Response, error) {
	u := fmt.Sprintf("repos/%v/%v/issues/%d", owner, repo, number)
	req, err := s.client.NewRequest("GET", u, nil)
	if err != nil {
		return nil, nil, err
	}

	// TODO: remove custom Accept header when this API fully launches.
	req.Header.Set("Accept", mediaTypeReactionsPreview)

	issue := new(Issue)
	resp, err := s.client.Do(req, issue)
	if err != nil {
		return nil, resp, err
	}

	return issue, resp, err
}

There are 2 places where we could set the context of the request. Either inside NewRequest, or inside Do.

After thinking about it, I think it should be done inside Do. It's more appropriate. A request can be created in advance of it being "done", and context is meant to be request scoped. So it's better to set it later rather than earlier. It's the approach that the golang.org/x/net/context/ctxhttp does and I think it's a good example. See here specifically, note that Get creates the *http.Request but doesn't set a context on it, it passes ctx to Do and Do sets the context on the request. This is the conclusion I came to after thinking a bit about it, but I welcome feedback.

If that's what we do, it would look like this:

func (s *IssuesService) Get(ctx context.Context, owner string, repo string, number int) (*Issue, *Response, error) {
	u := fmt.Sprintf("repos/%v/%v/issues/%d", owner, repo, number)
	req, err := s.client.NewRequest("GET", u, nil)
	if err != nil {
		return nil, nil, err
	}

	// TODO: remove custom Accept header when this API fully launches.
	req.Header.Set("Accept", mediaTypeReactionsPreview)

	issue := new(Issue)
	resp, err := s.client.Do(ctx, req, issue)
	if err != nil {
		return nil, resp, err
	}

	return issue, resp, err
}

// Do sends an API request and returns the API response.  The API response is
// JSON decoded and stored in the value pointed to by v, or returned as an
// error if an API error has occurred.  If v implements the io.Writer
// interface, the raw response body will be written to v, without attempting to
// first decode it.  If rate limit is exceeded and reset time is in the future,
// Do returns *RateLimitError immediately without making a network API call.
func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
	rateLimitCategory := category(req.URL.Path)

	// If we've hit rate limit, don't make further requests before Reset time.
	if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil {
		return nil, err
	}

	resp, err := c.client.Do(req.WithContext(ctx))
	if err != nil {
		// handle err
	}

	...

One potentially big problem is dealing with older versions of Go. 1.7 has context in standard library, but 1.6 and older would need to use golang.org/x/net/context package. That means we couldn't have the same context.Context in signature of each method without duplicating every file, once for 1.7+ importing context, and once for older importing golang.org/x/net/context.

We could use golang.org/x/net/context everywhere for now, since that package would be compatible with older Go versions.

For 1.6 and older, we'd either have to do what golang.org/x/net/context/ctxhttp does as far as using ctx on the HTTP request (since net/http of Go 1.6 doesn't have built-in support), or ignore ctx altogether. Best solution might be to simply use golang.org/x/net/context/ctxhttp package.

Anyway, that's enough planning for now until we hear back from @willnorris and @gmlewis.

[kevinburke]

I'm not sure 1.6 can handle it at all with the golang.org/x/ import or without because the http.Request object has no WithContext method in 1.6.

FWIW, here's how I handle this in my twilio-go library - I have an underlying rest client library that has NewRequest(method, path, body), and Do(request, v interface{}) where Do parses the response as JSON and decodes into the interface.

The twilio-go library has a function that calls rest.NewRequest, adds a context, then calls Do.

https://github.com/kevinburke/twilio-go/blob/master/http.go#L268-L291
https://github.com/kevinburke/twilio-go/blob/master/http_noctx.go
https://github.com/kevinburke/rest/blob/master/client.go#L81-L155

[dmitshur]

I'm not sure 1.6 can handle it at all with the golang.org/x/ import or without because the http.Request object has no WithContext method in 1.6.

It can be done by using http.Request.Cancel channel (which has existed for a while), it's just a lot of code. See how golang.org/x/net/context/ctxhttp package does it.

FWIW, here's how I handle this in my twilio-go library

Got it, thanks for a data point. 👍

[willnorris]

I'm not opposed to this change.

I'm also not opposed to just making the library require go1.7, though I'd probably want to wait until go1.8 is released so then we support "current version, plus one back". We'd document that if you need support for <1.7, then use X previous version.

Let's maybe start this work in a separate "context" branch?

[dmitshur]

I was thinking about that option too, and I really like it. Let's wait for 1.8 to come out (a matter of weeks), then we can update to use context package and require 1.7+ (which will be current plus previous).

We can make a branch or a tag with the last version that's compatible with Go 1.6 and older, but anyone who needs latest GitHub API features is likely to be using a modern version of Go as well.

Let's maybe start this work in a separate "context" branch?

Since this is a largely mechanical change, after deciding on the strategy, it seems unhelpful to start doing everything now since there are likely going to be changes before then (so it'd be more work in total). I'd be fine with making the decisions now, but starting implementation on a branch once 1.8 comes out.

We can make the change for one endpoint just to discuss how it looks. I'll make that PR.

[dmitshur]

Going with 1.7+ only gives us another option I haven't considered before.

We can either modify Do to accept ctx and set it internally, as I mentioned before:

@@ -396,7 +397,7 @@ func (c *Client) Rate() Rate {
 // interface, the raw response body will be written to v, without attempting to
 // first decode it.  If rate limit is exceeded and reset time is in the future,
 // Do returns *RateLimitError immediately without making a network API call.
-func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) {
+func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
        rateLimitCategory := category(req.URL.Path)
 
        // If we've hit rate limit, don't make further requests before Reset time.
@@ -404,7 +405,7 @@ func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) {
                return nil, err
        }
 
-       resp, err := c.client.Do(req)
+       resp, err := c.client.Do(req.WithContext(ctx))
        if err != nil {
                if e, ok := err.(*url.Error); ok {
                        if url, err := url.Parse(e.URL); err == nil {

And call it like this inside endpoint methods:

@@ -220,7 +221,7 @@ func (s *IssuesService) ListByRepo(owner string, repo string, opt *IssueListByRe
 // Get a single issue.
 //
 // GitHub API docs: http://developer.github.com/v3/issues/#get-a-single-issue
-func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Response, error) {
+func (s *IssuesService) Get(ctx context.Context, owner string, repo string, number int) (*Issue, *Response, error) {
        u := fmt.Sprintf("repos/%v/%v/issues/%d", owner, repo, number)
        req, err := s.client.NewRequest("GET", u, nil)
        if err != nil {
@@ -231,7 +232,7 @@ func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Res
        req.Header.Set("Accept", mediaTypeReactionsPreview)
 
        issue := new(Issue)
-       resp, err := s.client.Do(req, issue)
+       resp, err := s.client.Do(ctx, req, issue)
        if err != nil {
                return nil, resp, err
        }

Or, we can keep Do unmodified and instead add the context to the request inside each endpoint method. E.g.:

@@ -220,7 +221,7 @@ func (s *IssuesService) ListByRepo(owner string, repo string, opt *IssueListByRe
 // Get a single issue.
 //
 // GitHub API docs: http://developer.github.com/v3/issues/#get-a-single-issue
-func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Response, error) {
+func (s *IssuesService) Get(ctx context.Context, owner string, repo string, number int) (*Issue, *Response, error) {
        u := fmt.Sprintf("repos/%v/%v/issues/%d", owner, repo, number)
        req, err := s.client.NewRequest("GET", u, nil)
        if err != nil {
@@ -231,7 +232,7 @@ func (s *IssuesService) Get(owner string, repo string, number int) (*Issue, *Res
        req.Header.Set("Accept", mediaTypeReactionsPreview)
 
        issue := new(Issue)
-       resp, err := s.client.Do(req, issue)
+       resp, err := s.client.Do(req.WithContext(ctx), issue)
        if err != nil {
                return nil, resp, err
        }

I honestly don't know which is better. Any preferences?

I'm leaning towards modifying Do to accept ctx parameter.

[dmitshur]

Made #529, feel free to look and review the approach.

[dmitshur]

This was accidentally closed by unfortunate phrasing in #554:

This is part 1 of 3 PRs to resolve #526.

It's not resolved yet, so reopening.

[dmitshur]

For anyone watching this issue, the PR #529 that will resolve it is now completely finished and reviewed with approval. I will be merging it tomorrow (Sunday) morning.

/cc @kevinburke

[dmitshur]

@willnorris: We'd document that if you need support for <1.7, then use X previous version.

Where do you think such documentation would be a good fit?

796decd is the first commit that bumped library version and increased requirement to Go 1.7+. So people that need Go 1.4 through 1.6 support should use the parent commit, which is 82629e0.

Edit: We could mention that right after "go-github requires Go version 1.7 or greater." in README, is that the best thing to do in your opinion?

[kevinburke]

I believe "go get" respects git tags, so if you tag that parent commit with "go1.4", "go1.5", "go1.6", users with those Go versions will get that commit. I haven't verified this.

[dmitshur]

I don't think that's true for point versions, it only respects the go1 tag or branch. And as far as I know no one actually uses that feature, and there was proposal golang/go#15533 to consider removing it (I hope it'd go through, because IMO it doesn't add any value at this point, only mental complexity and overhead).

The most important rule is that if the local installation is running version "go1", get searches for a branch or tag named "go1". If no such version exists it retrieves the most recent version of the package.

Russ Cox: Actually, this is the only rule.

(Source: golang/go#15533 (comment).)

We can still make a go1.4 branch or tag, and simply document that it's what people should (manually) use if they need support for Go 1.4 through 1.6. Making it a branch makes it possible to backport some fixes there, but I don't think we'll be actively maintaining any such old version, given that master now supports "current plus one back".

That's the strategy we use in gopherjs repo, see https://github.com/gopherjs/gopherjs/branches (it's documented in the release blog posts). However, it's more meaningful there, because GopherJS always only supports the current point version of Go, not any older or newer.

[dmitshur]

As promised in #529 (comment), here's an update on the status of the PR #529. /cc @gmlewis

Something we should keep in mind when thinking this over is that our goal is to come up with the best public API and experience for the users of this library. The experience and burden for us, the library developers, is secondary to that.

Let me recap the current situation and the path undertaken in #529 so far.

I initially outlined in #526 (comment) that we have 2 options to choose from in terms of Do signature. We could either modify it to add an extra first parameter ctx context.Context, or we could rely on the caller to call pass the context via request (ala Do(req.WithContext(ctx), ...). For all other methods, we have no choice but to add ctx context.Context as first parameter, because there is simply no other way to resolve this issue.

When I first created #529 and then implemented all endpoints, I chose to go with Do(ctx context.Context, req *http.Request, v interface{}), as I originally planned without strong rationale:

I honestly don't know which is better. Any preferences?

I'm leaning towards modifying Do to accept ctx parameter.

However, after implementing all endpoints in #529, I realized that I thought it'd be better to keep signature of Do unmodified, as Do(req *http.Request, v interface{}), and pass context via the request.

The rationale I provided for that is written in 6b55015:

Pass context to Do via request, instead of as a separate parameter.

Do(req *http.Request, v interface{})

It seems cleaner because it eliminates the question of where to put
context. With the previous version that took 3 parameters, it was
possible (in theory) to pass the context via request or via first
parameter. Now, that ambiguity is gone, there's only one way.

It should be most helpful in situations where you have a request
that was made elsewhere, and you want to preserve its original context,
so you don't have to do something as awkward as:

ghClient.Do(req.Context(), req, ...)

Instead, it's simply:

ghClient.Do(req, ...)

Which was fine, and got approval to merge by @gmlewis.

However, today, I experimented with updating some code I had that uses go-github to the new API and see if that would lead to any insight. [1] [2] [3] [4] [5] For a good typical example, see https://github.com/shurcooL/notifications/pull/1/files.

That perspective led me to better understanding, and I now think that the better thing to do is to revert commit 6b55015 and use the following signature for Do after all:

Do(ctx context.Context, req *http.Request, v interface{})

Here are 4 reasons that make up the rationale/motivation for doing that:

1. First reason. After I switched to the new context branch of go-github locally and ran tests on my Go code that uses go-github, the compiler gave me very clear and actionable errors such as:

   # github.com/shurcooL/notifications/githubapi
   githubapi/githubapi.go:44: not enough arguments in call to s.clNoCache.Activity.ListNotifications
   	have (nil)
   	want (context.Context, *github.NotificationListOptions)
   githubapi/githubapi.go:53: not enough arguments in call to s.clNoCache.Activity.ListRepositoryNotifications
   	have (string, string, nil)
   	want (context.Context, string, string, *github.NotificationListOptions)
   githubapi/githubapi.go:151: not enough arguments in call to s.cl.Activity.ListRepositoryNotifications
   	have (string, string, nil)
   	want (context.Context, string, string, *github.NotificationListOptions)
   githubapi/githubapi.go:179: not enough arguments in call to s.cl.Activity.MarkThreadRead
   	have (string)
   	want (context.Context, string)
   githubapi/githubapi.go:193: not enough arguments in call to s.cl.Activity.MarkRepositoryNotificationsRead
   	have (string, string, time.Time)
   	want (context.Context, string, string, time.Time)
   FAIL	github.com/shurcooL/notifications/githubapi [build failed]
   

   It was very helpful and very easy to update my code to pass the additional context parameter. I had a great time doing it.

   However, I only later noticed that I forgot about the calls to Do method that some of my code made. That code continued to compile without any errors, and while it worked, the context wasn't propagated. Spotting that was hard, and worst of all, it felt very inconsistent.

   When every single method has an extra parameter, which makes the compiler help you catch instances of code you need to update, why doesn't that also apply to calls to Do method?

   Even after I updated my calls to Do to pass context with req.WithContext(ctx), it was harder to be confident I hadn't missed some cases. For example, imagine a situation where you set the context on a request earlier, and then call Do(req, ...). In the end, having an explicit first parameter for passing context is a lot easier to see that context is being propagated.

2. Second reason. I believe my rationale in commit 6b55015 is not valid. It definitely shouldn't carry as much weight as I originally thought.

   The situation I described where req is created in another function and then passed in... That just doesn't seem like something that would happen often. It seems that code that creates a NewRequest and then does Do is more likely. E.g., something like this.

3. Third reason. I originally noticed that between the two options of passing context separately and explicitly, and passing it via request, the latter was more in line with Go standard library.

   Clearly, the NewRequest and Do methods are modeled after same ones in net/http package:

   https://godoc.org/net/http#NewRequest
   https://godoc.org/net/http#Client.Do

   However, that's not evidence that it's the best way to pass context to Do. The net/http package API is frozen in Go1 and it couldn't have changed.

   A better place to look would be proposal: x/net/context: Context-aware HTTP helper functions. golang/go#11904, a proposal to make a friendlier context-aware http.Do which was resolved by creating the golang.org/x/net/context/ctxhttp package. /cc @bradfitz Look at its Do method:

   // Do sends an HTTP request with the provided http.Client and returns
   // an HTTP response.
   //
   // If the client is nil, http.DefaultClient is used.
   //
   // The provided ctx must be non-nil. If it is canceled or times out,
   // ctx.Err() will be returned.
   func Do(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) {
   	if client == nil {
   		client = http.DefaultClient
   	}
   	resp, err := client.Do(req.WithContext(ctx))
   	// If we got an error, and the context has been canceled,
   	// the context's error is probably more useful.
   	if err != nil {
   		select {
   		case <-ctx.Done():
   			err = ctx.Err()
   		default:
   		}
   	}
   	return resp, err
   }

   That was the outcome when the Go1 API freeze did not constrain the choice of Do's signature.

4. Fourth reason. There is a proposal x/tools: release static analysis tool for context propogation golang/go#16742 that tracks the creation of a tool to help with validating correct context propagation. Such a tool is hinted at in context package documentation, but so far does not exist.

   Had it existed and if it were trivial to verify that context is propagated and not accidentally dropped, this reason would not be included.

   The fact is that it's much easier for users to validate correct propagation of context if the signature of Do is changed to accept ctx context.Context explicitly as first parameter. So, this is additional evidence I believe we should go with what's friendlier to users of the API. As motivated by first reason, I believe it's friendlier to break the API of Do method than it is not to break it (somewhat counter-intuitively).

For these reasons, I think we should make the change of Do to:

Do(ctx context.Context, req *http.Request, v interface{})

In #529. I will do that now (by reverting 6b55015 in that PR). I'd like to hear from @gmlewis (and others who are familiar and have any insights or suggestions) if my rationale above is sound and if we are in agreement.

[gmlewis]

Thanks for the detailed explanation of your thought process, @shurcooL.
That makes total sense to me and I agree that Do should take ctx context.Context as its first argument.

[samuelkaufman]

Thanks for the documentation, I'm on Go 1.6 because I'm on Google AppEngine which is on 1.6. I was able to get to the right commit before the requirements switch due to these docs.

IMHO, treating 1.6 as unsupported seems a little hasty to me due to AppEngine being on 1.6, but as a non-contributor I'm happy to just pin at the old release until Google updates.