Support Google App Engine with recent context changes #582

gmlewis · 2017-03-06T21:26:52Z

Fixes #578.

Change-Id: Icc6b26877bcbc5e34845238c998d50a873fb7d65

Fixes google#578. Change-Id: Icc6b26877bcbc5e34845238c998d50a873fb7d65

dmitshur

Some questions, and minor suggestions. Let me know what you think.

dmitshur · 2017-03-07T23:35:43Z

github/without_appengine.go

+	"net/http"
+)
+
+func addContext(ctx context.Context, req *http.Request) (context.Context, *http.Request) {


Minor, but withContext seems like a better fitting name, given the two implementations call:

req.WithContext(ctx) appengine.WithContext(ctx, req)

dmitshur · 2017-03-07T23:36:35Z

github/github.go

@@ -390,7 +390,7 @@ func parseRate(r *http.Response) Rate {
 // The provided ctx must be non-nil. If it is canceled or times out,
 // ctx.Err() will be returned.
 func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
-	req = req.WithContext(ctx)
+	ctx, req = addContext(ctx, req)


Aren't we glad that we decided to add ctx parameter to Do, instead of doing Do(req.WithContext(ctx), ...) in all the call sites? 😆

Question about the path on App Engine. Again, I'm not familiar with how it works, but I'd like to understand for this PR.

Given that ctx is not really used, and addContext modifies only ctx, not req, does it actually have an effect on App Engine? Do you know how?

Yes, it has an effect on App Engine, and what App Engine does depends on what environment the code is run under.

Here is an example:
https://github.com/golang/appengine/blob/master/internal/api_classic.go#L56

Basically, it associates each App Engine request with a context so that logging (and other things) can be associated with a particular request.
When you are dealing with hundreds or thousands of queries per second, it is a really nice feature in the developers console to associate the log lines with the request that caused the them.
You can then mentally follow the flow of the single request.

I see. To make my question more specific, how does it have an effect given that the returned modified value of ctx is never used (aside from ctx.Done() and ctx.Err()) and never returned from Do?

Is it because the implementation of appengine.WithContext has side effects, other than just returning a modified copy of ctx?

dmitshur · 2017-03-07T23:39:32Z

github/with_appengine.go

+package github
+
+import (
+	"context"


I don't use App Engine, so I'm not familiar. If #526 (comment) is accurate, and App Engine uses/forces you to use Go 1.6, how does this work? Package context was added to Go standard library in Go 1.7, it wasn't available in Go 1.6.

That's an excellent point, and I believe this was an oversight on my part.
Internally, we have our own context so I didn't remember this was an issue.
I'll change it.

Although now that I'm thinking about this a little more, I'm wondering how the other import "context" in the rest of this package are going to work.

I think I'm going to have to do some more experimentation with this PR before we call it ready for review. Sorry, @shurcooL.

If that's the case, that's going to be an issue. Just about every single file in package github imports context.

https://github.com/google/go-github/blob/33bd2319b7816a4781538f/github/activity.go#L8

https://github.com/google/go-github/blob/33bd2319b7816a4781538f/github/activity_events.go#L9

https://github.com/google/go-github/blob/33bd2319b7816a4781538f/github/activity_notifications.go#L9

(60+ more files)

If we need the appengine version to import golang.org/x/net/context instead, we will have to make a copy of each and every .go file.

That would be a pretty significant disturbance, and it was one of the main reasons we decided to wait for Go 1.8 to come out, so that we could drop support for 1.6 and older before adding context support to this package.

Yes. I seem to remember hearing Dave Symonds say that the two implementations are 100% compatible... so maybe I don't need to change things after all... I'm just not sure if the Go 1.6 compiler will actually find the bare "context" import or not.
I definitely think that we don't need to make those kinds of accommodations here in this repo, though.

dmitshur · 2017-03-07T23:44:35Z

github/with_appengine.go

+
+// This file provides glue for making github work on App Engine.
+
+// +build appengine


Minor and has no effect, but I'd suggest swapping the order of these two lines, like so:

// +build appengine // This file provides glue for making github work on App Engine.

This is because I think the build constraints have higher priority and should be closer to top of the file. It's better for people to see the constraints as soon as possible, before the rest of the file. The only exception is that we place the copyright comment on top of that.

Somewhat of a precedent is here.

Change-Id: I5a1045cc3123da46597deee0ed79afac78db5a94

vvakame · 2017-03-15T15:29:27Z

How's it going? I need this change...

gmlewis · 2017-03-15T16:50:39Z

Sorry for the delay, @vvakame ... feel free to patch this into your code and try it out to see if it works while you are waiting for me to try it out myself. I've had some other pressing issues I'm attending to.

gmlewis · 2017-03-15T17:43:27Z

After discussing this with @willnorris a bit, we are thinking that you will need to run the following command to make this run with the current version of Go on App Engine Classic (which as of this writing uses Go 1.6):

gofmt -w -r '"context" -> "golang.org/x/net/context"' *.go

This will rewrite all the context imports so that it should compile with Go 1.6.

We do not plan on supporting multiple versions of the Go compiler in this package, so we may simply have to add instructions to the with_appengine.go file on how to perform this transformation as long as App Engine Classic has not moved to Go 1.8.

Change-Id: Icc53cb08370aaf86ada3656f4b5a7ee72881b6d4

dmitshur

@gmlewis That makes sense.

I had some hope there was another solution after seeing @quentinmit's golang/go#19208 (comment), but he hasn't replied and it sounds like the "tiny patch" he was referring to is likely the same solution as this PR offers (which requires code modification).

Two thoughts (see inline comments), but this looks good. I'll give it a final review after you reply.

dmitshur · 2017-03-15T18:39:15Z

github/with_appengine.go

+import (
+	"net/http"
+
+	"golang.org/x/net/context"


Since all other files import "context", and the gofmt -w -r '"context" -> "golang.org/x/net/context"' *.go step is necessary, is there any advantage to importing "golang.org/x/net/context" here instead of "context"?

In the end, it doesn't matter, but I'm leaning slightly towards preferring "context" because it's more consistent. Either "context" package doesn't exist, and the rewrite is neccessary everywhere, or it does exist, and nothing needs to be done anywhere. It would avoid importing both types of context package if "context" happens to be (or becomes) available.

I don't feel too strongly about this, so feel free to go with whatever you think is best.

Excellent point.
Done.

dmitshur · 2017-03-15T18:42:06Z

github/with_appengine.go

+// Go 1.6, you will need to rewrite all the import "context" lines.
+// Fortunately, this is easy with "gofmt":
+//
+//     gofmt -w -r '"context" -> "golang.org/x/net/context"' *.go


Asking users to modify the source code of a package in order to get it to work is pretty unusual, and I feel this comment would be hard to spot.

I think a nice thing we can do for users is mention this in a new "App Engine" section of README (and package comment, to keep them in sync). It can simply state the current situation, that the package is expected not to build by default and needs to be modified, and point to this file for further instructions. Similar to what you wrote in #582 (comment).

SGTM.
Done.

Change-Id: I036206375e3a03c55559bdb60fb7fdef26c5d074

dmitshur

LGTM.

samuelkaufman · 2017-03-16T13:43:16Z

Awesome! Thanks guys, I'll try and test this out today. I'm using godep already so formatting the code in the vendor dir and adding the gofmt command to my build process should not be hard.

* Support Google App Engine with recent context changes Fixes google#578.

Support Google App Engine with recent context changes

ee6d701

Fixes google#578. Change-Id: Icc6b26877bcbc5e34845238c998d50a873fb7d65

dmitshur requested changes Mar 7, 2017

View reviewed changes

Address review feedback

767d9b0

Change-Id: I5a1045cc3123da46597deee0ed79afac78db5a94

dmitshur mentioned this pull request Mar 14, 2017

build: breaking change in go-github golang/go#19208

Closed

Add instructions for compiling on App Engine Classic

e150f4d

Change-Id: Icc53cb08370aaf86ada3656f4b5a7ee72881b6d4

dmitshur reviewed Mar 15, 2017

View reviewed changes

Address review feedback

8971354

Change-Id: I036206375e3a03c55559bdb60fb7fdef26c5d074

dmitshur approved these changes Mar 15, 2017

View reviewed changes

gmlewis merged commit 7a71e0e into google:master Mar 15, 2017

gmlewis deleted the i-578-app-engine branch March 15, 2017 20:52

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Support Google App Engine with recent context changes (google#582)

c59749c

* Support Google App Engine with recent context changes Fixes google#578.

ianrose14 mentioned this pull request Aug 15, 2017

Using go-github on Google App Engine with recent context changes - any interest? #578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Google App Engine with recent context changes #582

Support Google App Engine with recent context changes #582

gmlewis commented Mar 6, 2017

dmitshur left a comment

dmitshur Mar 7, 2017

dmitshur Mar 7, 2017

dmitshur Mar 7, 2017

gmlewis Mar 8, 2017

dmitshur Mar 8, 2017

dmitshur Mar 7, 2017

gmlewis Mar 8, 2017

gmlewis Mar 8, 2017

dmitshur Mar 8, 2017 •

edited

Loading

gmlewis Mar 8, 2017

dmitshur Mar 7, 2017

vvakame commented Mar 15, 2017

gmlewis commented Mar 15, 2017

gmlewis commented Mar 15, 2017

dmitshur left a comment •

edited

Loading

dmitshur Mar 15, 2017

gmlewis Mar 15, 2017

dmitshur Mar 15, 2017

gmlewis Mar 15, 2017

dmitshur left a comment

samuelkaufman commented Mar 16, 2017


		// This file provides glue for making github work on App Engine.

		// +build appengine

Support Google App Engine with recent context changes #582

Support Google App Engine with recent context changes #582

Conversation

gmlewis commented Mar 6, 2017

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvakame commented Mar 15, 2017

gmlewis commented Mar 15, 2017

gmlewis commented Mar 15, 2017

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

samuelkaufman commented Mar 16, 2017

dmitshur Mar 8, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading